Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIRRTL][GCT] Add support for tapping non-passive bundles, add test. #4815

Merged
merged 1 commit into from
Mar 29, 2023

Conversation

dtzSiFive
Copy link
Contributor

@dtzSiFive dtzSiFive commented Mar 13, 2023

Builds on #4801 .

Relevant commit: c6a7974 .

@dtzSiFive
Copy link
Contributor Author

Only draft until support piece is in, FWIW.

@dtzSiFive dtzSiFive requested a review from seldridge March 13, 2023 19:04
@dtzSiFive dtzSiFive marked this pull request as ready for review March 28, 2023 22:12
@dtzSiFive dtzSiFive force-pushed the feature/tap-non-passive-bundle branch from c6a7974 to aacaf91 Compare March 28, 2023 22:13
@dtzSiFive
Copy link
Contributor Author

Idea is to support what could also be done w/probes. Anyway, this would introduce a difference in behavior regarding what's connectable with and without the hidden --lower-annotations-no-ref-type-ports option enabled, which presently allows taps to be bidirectional:

circuit WiringFlips : %[[
  {
    "class": "sifive.enterprise.grandcentral.DataTapsAnnotation",
    "keys": [
      {
        "class": "sifive.enterprise.grandcentral.ReferenceDataTapKey",
        "source": "~WiringFlips|WiringFlips/c:Child>w",
        "sink": "~WiringFlips|WiringFlips>w"
      }
    ]
  }
]]
  module Child :
     output x : UInt<3>
     wire w : { flip x : UInt<3>, y : UInt<4> }
     w.y <= UInt(3)
     x <= w.x

  module WiringFlips :
     input x : UInt<3>
     output y : UInt<4>
     output c_x : UInt<3>

     wire w : { flip x : UInt<3>, y : UInt<4> }
     w.x <= x
     y <= w.y

     inst c of Child
     c_x <= c.x

This probably should be rejected for DataTap's, may make sense for LegacyWiringProblem's that don't have multiple sinks (ambiguous which sink's flipped fields should drive the source)?

@dtzSiFive
Copy link
Contributor Author

Welp, we presently crash on the above input w/o the hidden flag, so ... progress!

@@ -841,6 +841,14 @@ LogicalResult LowerAnnotationsPass::solveWiringProblems(ApplyState &state) {
return failure();
}
}
// If wiring using references, check that the sink value we connect to is
// passive.
Copy link
Contributor Author

@dtzSiFive dtzSiFive Mar 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can totally support some non-passive sinks (driving all fields individually) in the future, at least as long as it's not a port or something. Don't think it's worth supporting until there's a need/ask.

@dtzSiFive dtzSiFive merged commit 5adab65 into llvm:main Mar 29, 2023
@dtzSiFive dtzSiFive deleted the feature/tap-non-passive-bundle branch March 29, 2023 19:07
dtzSiFive added a commit that referenced this pull request Apr 5, 2023
Test introduced in #4815.

1) CHECK-DAG-SAME doesn't exist/work.
2) Test check line was wrong anyway.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant