[FIRRTL] parser: support rwprobe of open-agg instance results.#10018
[FIRRTL] parser: support rwprobe of open-agg instance results.#10018
Conversation
Refactor aggregate handling to common helper.
Don't require the entire instance result is forceable. Add test that fails before this change.
|
For PRs that are stacked, you can use the "base" (or the |
| // Create bounce wire for the instance result. | ||
| // This may be an open aggregate, or other non-base type. |
There was a problem hiding this comment.
Mega-nit... I usually prefer line wrapping for paragraphs.
| wire inst_rcoa_a : RWProbe<UInt<1>> | ||
| wire inst_rcoa_passthru : RWProbe<UInt<1>> |
There was a problem hiding this comment.
Nit: what is the rc in rcoa? I assume the latter two characters are "open aggregate".
There was a problem hiding this comment.
Nit: Consider moving the definitions of these wires closer to their usage sites.
| ; CHECK-NEXT: %[[INST_RCOA_A:[^ ]+]] = firrtl.wire : !firrtl.rwprobe<uint<1>> | ||
| ; CHECK-NEXT: %[[INST_RCOA_PASSTHRU:[^ ]+]] = firrtl.wire : !firrtl.rwprobe<uint<1>> |
There was a problem hiding this comment.
Nit: these wires should have predictable names? Is the regex capture necessary?
There was a problem hiding this comment.
I mean it's all predictable until it's is broken by something?
If we want policy of assuming declaration names are stable enough that works for me.
The names make things fair bit easier to read that's for sure 👍.
There was a problem hiding this comment.
Yeah, emphasize readability of the test. It's fine to assume that ImplicitSSAName will not break.
Looking at this with fresh eyes, it may be good to try to prune this test to its core essence. Is there anything here which is duplicated by previous tests that could be ignored so that this could be slimmed down? I'm also now questioning if we should, in a follow-on, add an opensubfield cache to avoid the duplicated subfields.
I gave the MLIR Testing Guide 1 a quick read to see if there was any guidance about SSA naming, but was more drawn to some of the other ideas which prompted the question above.
| ; CHECK-NEXT: %[[RCOA_IN_A:.+]] = firrtl.opensubfield %[[RCOA_IN]][a] | ||
| ; CHECK-NEXT: %[[RCOA_IN_BOUNCE_A2:.+]] = firrtl.opensubfield %[[RCOA_IN_BOUNCE]][a] | ||
| ; CHECK-NEXT: firrtl.matchingconnect %[[RCOA_IN_A]], %[[RCOA_IN_BOUNCE_A2]] |
There was a problem hiding this comment.
Mega-nit: these checks are using a capture format that is different from the prior ones: .+ vs. [^ ]+.
| ; CHECK-NEXT: firrtl.ref.define %[[INST_RCOA_PASSTHRU]], %[[RCOA_IN_BOUNCE_RW]] | ||
| define inst_rcoa_a = rwprobe(rcoa.in.a) | ||
| define inst_rcoa_passthru = rcoa.in.rw | ||
| connect rcoa.in.a, UInt<1>(0) |
There was a problem hiding this comment.
Nit: connect is technically unnecessary for parsing. I can see having this so that this could theoretically be compiled to Verilog.
Thanks, yeah that's neat. Unfortunately I don't think that works if the branches aren't on the repo PR'ing to. Mostly because if I target the other PR branch it wouldn't be an llvm/circt PR anymore (don't think it'll let me) And by the time was drafting this PR text and noticed it didn't seem worth rejiggering (or close/open) to get that to work. Old habits re:using separate repo for my dev/PR branches... 😅 |
Ah, that's unfortunate. Yeah, I don't think that will work, then. |
2 participants
Don't require the entire instance result is forceable.
Add test that fails before this change.
Builds on #10017 .