-
Notifications
You must be signed in to change notification settings - Fork 389
[Comb] Add canonicalization for comb.truth_table #9214
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
base: main
Are you sure you want to change the base?
Conversation
bacdbbc to
5ff85a1
Compare
5ff85a1 to
0091687
Compare
|
Thanks! The implementation and tests look very nice. I'm just not sure whether this really should be a canonicalization pattern. Considering the exponential growth of truth tables this could very easily have a massive effect on runtime . And it doesn't make much sense to apply this pattern repeatedly in the canonicalization convergence loop since it only depends on the truth table itself. I think we should move this to a dedicated pass instead. WDYT @uenoku? |
uenoku
left a comment
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.
Cool, thank you for adding this! LGTM!
I think that's fair concern (though the table size is already 2^n so i don't think it's going to be a critical regression). Maybe we can use the same pattern for canonicalization and dedicated pass and for canonicalization it might makes sense to restrict the lut size to small (e.g. 4 inputs). WDYT? @fzi-hielscher Alternatively we can also disable the canonicalization for negation and simply find identity patterns |
|
Yeah, not touching truth tables beyond a certain limit sounds sensible to me. That being said, I don't have an idea what sizes we are dealing with in practice and what a "reasonable" limit would be. |
|
We've been talking about introducing a '-O1' pass for the core dialect as we're definitely abusing canonicalizers for some of them. If there's any debate, this would be a good candidate for having in that pass. |
Adds canonicalization patterns and tests for
comb.truth_table:hw.constantcomb.truth_tableCloses #8925