Repository navigation
[OptimizeInstructions] Generalize low-bit masks - #9293
Conversation
Extend visitBinary's low-bit mask elimination and narrow-load unsigned conversion from i32.and to both i32.and and i64.and using Match::binary(Abstract::And, ...). Also replace the contiguous 2^k - 1 mask check with Bits::lowBitMask64(getMaxBits(left)) so non-contiguous masks that preserve all active low bits of the LHS are eliminated as well.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors and generalizes the bitmask optimization logic in OptimizeInstructions.cpp to support both 32-bit and 64-bit masks. It introduces a new lowBitMask64 helper in bits.h and uses pattern matching to simplify redundant masks that preserve all possible non-zero low bits of an expression. Additionally, it cleans up overlapping optimization rules and adds comprehensive test coverage in optimize-instructions-mvp.wast. There are no review comments, so I have no additional feedback to provide.
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
lgtm though I do think this kind of thing is low priority. We can probably get larger wins from more general approaches, like say integrating LLVM's instcombine.
|
That might have been plausible back when it would have been infeasible to figure out what the gap between OptimizeInstructions and instcombine was. Now that we can just have Gemini cross-reference OptimizeInstructions with LLVM's instcombine to find missing opportunities, I think the maintenance burden of an updated standalone OptimizeInstructions will be much lower than a new integration with LLVM. I agree this is lower priority than many of the other promising projects we have in the works, though. My plan for now is to upload just one of these a day to avoid taking time away from other projects. Does that sound reasonable to you? |
|
One a day sounds reasonable. But rather than cross-reference InstCombine... how about using it? Literally take those source files and build a shim around them. The shim would be the only thing to verify, and we'd get everything at once. |
|
My intuition is that the shim would be far more code and complexity than its worth, but I'd be happy to be proven wrong about that. Probably something we can prototype "for free." |
Extend visitBinary's low-bit mask elimination and narrow-load unsigned conversion from i32.and to both i32.and and i64.and using Match::binary(Abstract::And, ...). Also replace the contiguous 2^k - 1 mask check with Bits::lowBitMask64(getMaxBits(left)) so non-contiguous masks that preserve all active low bits of the LHS are eliminated as well.