-
Notifications
You must be signed in to change notification settings - Fork 314
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
[Verif] Add StripContracts pass #8144
Conversation
getOperation()->walk([](ContractOp op) { | ||
op.replaceAllUsesWith(op.getInputs()); | ||
op.erase(); | ||
}); |
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.
Pretty degenerate case but does this work in the following case?
%0 = verif.contract %1 : i32 {}
%1 = verif.contract %0 : i32 {}
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.
Good thinking. Should work. Let me add this to the tests.
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.
Actually needed a tweak to avoid an assert in replaceAllUsesWith
and to allow op.erase
to work if the operation is cycle.
0ae9834
to
86c1ef0
Compare
86c1ef0
to
1c83d54
Compare
hw.module @bar() { | ||
// CHECK-NOT: verif.contract | ||
%0 = verif.contract %1 : i42 {} | ||
%1 = verif.contract %0 : i42 {} |
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's actually the semantics if we return %1 as an output of the module? Also, the pass would still crash in that case, right? Not really important to fix here though. We have similar issues with comb.
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.
That's a great point, that would still crash. It would be better if it didn't, though. I think in that case we'd want to just not remove the contract. But the semantics are the same as in %0 = comb.add %0, %0; hw.output %0
-- hard to define. Maybe some undefined
or poison
value, or unknown
.
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.
Fixed this for now by just not removing cyclical contracts.
Add a simple pass that removes all contracts from the IR, treating them as passthroughs. Run the pass in firtool's HW-to-SV pipeline early on to add additional optimization opportunities. ExportVerilog will eventually ignore contracts anyway.
1c83d54
to
cb6bd01
Compare
Add a simple pass that removes all contracts from the IR, treating them as passthroughs. Run the pass in firtool's HW-to-SV pipeline early on to add additional optimization opportunities. ExportVerilog will eventually ignore contracts anyway.