-
Notifications
You must be signed in to change notification settings - Fork 281
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] Fix dominance issue in GroupResetsAndEnables #5511
Conversation
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.
Thanks for fixing this!
@@ -166,7 +165,8 @@ struct GroupAssignmentsInIfPattern : public OpRewritePattern<scf::IfOp> { | |||
// Skip anything not in a ClockTreeOp | |||
auto clockTreeOp = ifOp->getParentOfType<ClockTreeOp>(); | |||
if (!clockTreeOp) | |||
return failure(); | |||
// This probably means the design uses a derived clock (warning). | |||
return success(false); |
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.
This is exactly equivalent to return failure()
(see implementation in LogicalResult.h), so no need to change.
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.
Oh thanks, I ran into an issue where the compiler was failing without displaying an error during debugging and thought it was because of this, but it must have been something else related to the dominance stuff. I undid that change, thanks for the catch!
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.
I see. That might be because no failure is returned in line 207 (also no error message). Thanks for the catch!
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.
Should be fixed in 227ae28
This fixes a dominance issue that can occur in GroupResetsAndEnables. Unfortunately I haven't been able to reduce the test case from a larger design (tried using circt-reduce).
There are two fixes:
definition->getUsers()
to check for dominance, sincedefinition
is the operation that will actually be moved. If it generates multiple results, the place it is being moved to must dominate all those results, not just the operand used in the block.