[PATCH] D152568: [InstCombine] Transform `(binop1 (binop2 (lshift X,Amt),Mask),(lshift Y,Amt))`

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n marked an inline comment as done.
goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2006
+  if (I.getOpcode() != Instruction::And && BWOpc != Instruction::And)
+      return nullptr;
+
----------------
nikic wrote:
> So I guess this works, but it's mostly by accident and doesn't capture the core requirement. We're moving a bitwise op before the shift, which for and/or requires that the shifted out bits are zero (this also extends to add/sub): https://alive2.llvm.org/ce/z/ieAz2T This requirement doesn't exist for and, which is why this check happens to work.
> 
> I would prefer checking that the constant round-trips the shift rather than checking opcodes here. It's okay to enforce this for `and` as well, as demanded bits simplification will zero out the low bits.
Okay, updated for all the cases I could think of.

I didn't extend to `sub` just because 1) it has some special cases and 2) `sub X, Const` always canonicalizes to `add X, -Const` so covering add is enough.

Current workings are:
if both ops are same -> do generic transform and just re-associate the shift. (if `add`, then only if `shl`)
if both are bitwise and 1 is `and` -> just do transform
if both are bitwise no and -> check mask -> do transform
if outer binop is and -> just do transform
else -> fail


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152568/new/

https://reviews.llvm.org/D152568