[firrtl] Move LowerLayers after LowerXMR #8405
Conversation
772757e to
38b6bef
Compare
|
The changes in this PR introduce a lot of unreachable code in |
f3ae7e8 to
d3c4abb
Compare
febc89c to
3057138
Compare
1bcc6d0 to
4319a18
Compare
| if (type_isa<FStringType>(operand.getType())) { | ||
| localBuilder.setInsertionPoint(op); | ||
| replacement = localBuilder.clone(*operand.getDefiningOp())->getResult(0); | ||
| replacements.insert({operand, replacement}); | ||
| return replacement; 8000 | ||
| } |
There was a problem hiding this comment.
You are replacing operand with result 0, when it should match the original result number, which you can only get if you check that operand is an OpResult. If operand is a block argument then this should fail gracefully. It doesn't seem like there is a reachable bug here, but I think it would be easier to understand if it was written more defensively.
|
40a9685 fixes a bug where when capturing an instance port, a node needs to be created. This matches the behavior of LowerXMR. The buggy behavior before this is that this would XMR directly to the instance which is just wrong. |
90ab8c5 to
6927b0a
Compare
Move the `LowerLayers` pass after the `LowerXMR` pass. To do this, all passes at the end of the CHIRRTL to Low FIRRTL pipeline are moved after `LowerXMR`. This is necessary because the `LowerLayers` pass cannot, at present, be moved after the passes at the end of the pipeline. This is done to enable forcing out of layers. By lowering probes to XMRs, the layers can be lowered trivially to modules/instances and their XMRs will now (seemingly) magically just work. By doing the loweirng in this way, it avoids ever having to represent an input probe in the FIRRTL dialect. A consequence of this is that there are now simplifying assumptions (preconditions) that can be made about the `LowerLayers` pass: 1. It will never see a number of probe ops because the `LowerXMR` pass has a postcondition that all of these are removed. 2. Input and output ports can never be created on modules created from layer blocks. While I am generally always in favor of passes being relocatable anywhere in the pipeline, this is one pass that really does _not_ make sense to be relocatable. In effect, this pass is part of the lowering from FIRRTL to HW. There is no point in being able to use it earlier in the pipeline. That said, it still is tested to work with things which we may one-day preserve, like `WhenOp`s. Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
6927b0a to
9425597
Compare
|
Thanks for the review @rwy7. I will merge this after running some internal tests. I'm slating to get this into the 1.115.0 release this week. |
|
I have this passing regressions internally, though cherry-picked onto the 1.114.1 tag. |
Move the
LowerLayerspass after theLowerXMRpass. To do this, allpasses at the end of the CHIRRTL to Low FIRRTL pipeline are moved after
LowerXMR. This is necessary because theLowerLayerspass cannot, atpresent, be moved after the passes at the end of the pipeline.
This is done to enable forcing out of layers. By lowering probes to XMRs,
the layers can be lowered trivially to modules/instances and their XMRs
will now (seemingly) magically just work. By doing the loweirng in this
way, it avoids ever having to represent an input probe in the FIRRTL
dialect.
A consequence of this is that there are now simplifying
assumptions (preconditions) that can be made about the
LowerLayerspass:It will never see a number of probe ops because the
LowerXMRpass hasa postcondition that all of these are removed.
Input and output ports can never be created on modules created from
layer blocks.
While I am generally always in favor of passes being relocatable anywhere
in the pipeline, this is one pass that really does not make sense to be
relocatable. In effect, this pass is part of the lowering from FIRRTL to
HW. There is no point in being able to use it earlier in the pipeline.
That said, it still is tested to work with things which we may one-day
preserve, like
WhenOps.Todo