-
Notifications
You must be signed in to change notification settings - Fork 285
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] Infer StateOp reset from CompReg #4997
Merged
Merged
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
78bdeb0
Copy reset from CompReg to StateOp (reset currently unchecked)
TaoBi22 b52305b
Add checks that the reset value is valid
TaoBi22 08ca1c3
Update comments
TaoBi22 c31be99
clang-format
TaoBi22 dbd7031
Update to error on reset inference when StateOp already has a reset
TaoBi22 fda7273
Fix non-trivial reset assignment
TaoBi22 0f88cd5
Add compreg reset inference integration test
TaoBi22 2197394
Return on multiple reset error
TaoBi22 185343f
clang-format
TaoBi22 8d77815
Move inference test into ConvertToArcs tests and remove unnecessary C…
TaoBi22 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
clang-format
- Loading branch information
commit 185343f6d3c6863b08ed3b0297dd17327c7f338a
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if there is already a reset present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you happy for this just to be covered with an assertion? I suppose we could
comb.or
multiple reset lines, but I'm unable to think of a situation where you would expect/desire a reset to already be present on the StateOp at this point, unless there are additions you have planned.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The situation/assertion could be triggered with an input that has both state ops and compreg ops in there which does not happen currently. But the fact that a user can construct such an IR is enough of a reason that we shouldn't use an assertion. However, I think it's not necessary to support this case right now, so I would just go for an
emitError
in that case as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah of course! Sounds good, I'll do that :)