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][LowerLayers] Remove handling of some ref ops #7640

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

rwy7
Copy link
Contributor

@rwy7 rwy7 commented Sep 26, 2024

Remove code that handles the capturing of ref cast and ref send operands specially, since this code is doing only what the generic capture-handling code does just a few lines below.

There is a bug in the handling of ref.cast which is removed by this change: We are accidentally checking that the cast op itself is within the layerblock, when we should actually be checking if its operand is defined within the layerblock. This causes us to skip converting the captured operand of a ref.cast op into a port. The generic codepath works fine.

Remove code that handles the capturing of ref cast and ref send operands specially, since
this code is doing only what the generic capture-handling code does just a few lines below.

There is a bug in the handling of ref.cast which is removed by this change: We
are accidentally checking that the cast op itself is within the layerblock,
when we should actually be checking if its operand is defined within the
layerblock. This causes us to incorrectly capture the operand of a ref.cast op.
The generic codepath works fine.
@rwy7 rwy7 requested review from dtzSiFive and seldridge and removed request for seldridge and darthscsi September 26, 2024 18:55
@rwy7 rwy7 merged commit 6e4e866 into llvm:main Sep 30, 2024
4 checks passed
Copy link
Contributor

@dtzSiFive dtzSiFive left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@rwy7 rwy7 deleted the fix-lower-layers branch September 30, 2024 13:36
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.

2 participants