Fix CSESubAccesses for SubAccesses with flips #2112
Conversation
jackkoenig
force-pushed
the
fix-csesubaccesses
branch
from
56e9cfa
to
786be1e
Compare
| expr.foreach(onExpr(outer)) | ||
| // Stop recursing on any non-Source because flips can make the SubAccess a Source despite the | ||
| // overall Expression being a Sink | ||
| if (flow(expr) == SourceFlow) expr.foreach(onExpr(outer)) |
There was a problem hiding this comment.
To check my understanding... the implication of this is that we won't CSE all subaccesses, and punt on ones which have weird internal flippedness?
I think this is totally fair.
There was a problem hiding this comment.
This comment is really talking about how an Expression that is a Sink may have a SubAccess that is a SourceFlow, eg. from the added tests:
input out : { flip foo : UInt<8> }[2]
...
out[idx].foo <= in.foo
out[idx].foo is a sink, but the out[idx] SubAccess is SourceFlow, the flip on foo is what makes the larger Expression a sink.
Note that this case probably should not ever be handled by this transform, or at least, it would require a lot more sophistication to handle write ports
That being said, this check also stops handling any SubAccesses of bidirectional Vecs. Technically, this transform could handle them if running after ExpandConnects, but it's a little more complicated and I think there's a better way to do it than complicating the logic in this transform, see original PR description. From the other test I added in this PR:
output out : { flip foo : UInt<8>, bar : UInt<8> }[4]
...
out[idx] <= in"
The flow of a LHS SubAccess node may still be SourceFlow if the type of the Vec element has a flip. Tweak the logic of CSESubAccesses to check every Expression flow while recursing instead of just the flow of the final SubAccess. Co-authored-by: Schuyler Eldridge <[email protected]>
jackkoenig
force-pushed
the
fix-csesubaccesses
branch
from
8998c31
to
5410753
Compare
The flow of a LHS SubAccess node may still be SourceFlow if the type of the Vec element has a flip. Tweak the logic of CSESubAccesses to check every Expression flow while recursing instead of just the flow of the final SubAccess. Co-authored-by: Schuyler Eldridge <[email protected]> Co-authored-by: Schuyler Eldridge <[email protected]> (cherry picked from commit ed1eb88)
mergify
bot
added
the
Backported
The flow of a LHS SubAccess node may still be SourceFlow if the type of the Vec element has a flip. Tweak the logic of CSESubAccesses to check every Expression flow while recursing instead of just the flow of the final SubAccess. Co-authored-by: Schuyler Eldridge <[email protected]> Co-authored-by: Schuyler Eldridge <[email protected]> (cherry picked from commit ed1eb88) Co-authored-by: Jack Koenig <[email protected]>
Fixes a bug in #2099 that is causing Chisel CI to fail.
The flow of a LHS SubAccess node may still be SourceFlow if the type of
the Vec element has a flip. Tweak the logic of CSESubAccesses to check
every Expression flow while recursing instead of just the flow of the
final SubAccess.
This did highlight a shortcoming of
CSESubAccesses. Basically, the transform uses nodes to avoid quadratic behavior ofRemoveAccessesbecause nodes are unaffected byExpandConnectsand thus it only lowers theSubAccessonce. See #643 for more details on that issue. The shortcoming is that nodes cannot be used to represent bidirectional connections, soCSESubAccessesfundamentally cannot handle dynamic indexing of bidirectional Vecs. I did try, but it doesn't really work with the current approach.I think a better approach is to fix the quadratic behavior in
RemoveAccessesitself, then haveCSESubAccessesuse wires instead of nodes so that it can also handle bidirectional Vecs. Future work!Contributor Checklist
No need for release notes, CSESubAccesses is not yet released.
Type of Improvement
API Impact
No impact
Backend Code Generation Impact
No impact
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
Please Merge?