Fix optimization rules for narrow types: wrap i8 results to 8 bits.#5409
Fix optimization rules for narrow types: wrap i8 results to 8 bits.#5409cfallin merged 3 commits intobytecodealliance:mainfrom
Conversation
This fixes bytecodealliance#5405. In the egraph mid-end's optimization rules, we were rewriting e.g. imuls of two iconsts to an iconst of the result, but without masking off the high bits (beyond the result type's width). This was producing iconsts with set high bits beyond their types' width, which is not legal. In addition, this PR adds some optimizations to the algebraic rules to recognize e.g. `x == x` (and all other integer comparison operators) and resolve to 1 or 0 as appropriate.
7b542cd to
5b24df8
Compare
jameysharp
left a comment
There was a problem hiding this comment.
Other than the one comment, this looks right to me!
I thought backends were supposed to mask it. I believe cg_clif sometimes sign extends immediates. |
Hmm, it seems we discussed this and filed #3059 a while ago, and haven't done any thinking about it since. I suspect that we aren't doing such masking everywhere we need to be, and requiring rules everywhere to do the masking (e.g., const-folding an For now I guess we can keep the status quo, but we should think about this some more (on that issue probably)! |
|
BTW I'm curious if the new optimization rules have any impact on any benchmarks. Do these patterns show up in any CLIF we're generating from wasm, or do our current benchmarks compile to the same sequence of instructions either way? Not terribly important but if this is a significant win for some reason, that'd be nice to know. |
|
That's a good question, re: the |
|
And, to follow up, a quick smoketest with |
This PR follows up on #5382 and #5391, which rebuilt the egraph-based optimization framework to be more performant, by enabling it by default. Based on performance results in #5382 (my measurements on SpiderMonkey and bjorn3's independent confirmation with cg_clif), it seems that this is reasonable to enable. Now that we have been fuzzing compiler configurations with egraph opts (#5388) for 6 weeks, having fixed a few fuzzbugs that came up (#5409, #5420, #5438) and subsequently received no further reports from OSS-Fuzz, I believe it is stable enough to rely on. This PR enables `use_egraphs`, and also normalizes its meaning: previously it forced optimization (it basically meant "turn on the egraph optimization machinery"), now it runs egraph opts if the opt level indicates (it means "use egraphs to optimize if we are going to optimize"). The conditionals in the top-level pass driver are a little subtle, but will get simpler once we can remove the non-egraph path (which we plan to do eventually!). Fixes #5181.
This fixes #5405.
In the egraph mid-end's optimization rules, we were rewriting e.g. imuls of two iconsts to an iconst of the result, but without masking off the high bits (beyond the result type's width). This was producing iconsts with set high bits beyond their types' width, which is not legal.
In addition, this PR adds some optimizations to the algebraic rules to recognize e.g.
x == x(and all other integer comparison operators) and resolve to 1 or 0 as appropriate.