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

[Arc] Hoist reset value in CompReg when lowering for simulation #6972

Merged
merged 4 commits into from
May 2, 2024

Conversation

Moxinilian
Copy link
Member

Arcilator currently does not support non-zero reset values for compreg. This change canonicalizes reset values to a mux on the input, as it currently makes no difference for simulation purposes, and sidesteps the reset value limitations.

@maerhart
Copy link
Member

As you pointed out, the ConvertToArcs pass supports zero-resets by adding them as operands to the arc.state operations. This patch removes zero-resets as well, thus relying on InferStateProperties to detect and add it again.

Should we instead only apply this pattern if the reset value is non-zero?

@Moxinilian
Copy link
Member Author

I updated it accordingly. I used a pattern match to check for constant zero instead of the naive hw.constant check in Arcilator in case canonicalization patterns next to the CompRegCanonicalizer would still need to be ran to apply simplification.

Copy link
Member

@maerhart maerhart 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 for adding support!

If you want to be very thorough you could add a very small integration test with just a compreg that has a non-zero reset value since this patch assumes that the arc canonicalizer will always be run before ConvertToArcs. So, if someone changes the Arcilator pipeline, the loss of support could go unnoticed.

@Moxinilian
Copy link
Member Author

I wanted to do that and forgot about it! Good idea.

@Moxinilian Moxinilian requested a review from maerhart May 1, 2024 18:46
Copy link
Member

@maerhart maerhart left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@Moxinilian Moxinilian merged commit 523d6fd into llvm:main May 2, 2024
4 checks passed
@Moxinilian Moxinilian deleted the non-zero-reset branch May 2, 2024 09:14
@fabianschuiki
Copy link
Contributor

Very cool, thanks for adding this! 🥳

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.

3 participants