[PATCH] D152568: [InstCombine] Transform `(bitwise1 (bitwise2 (lshift1 X, C), C1), (lshift2 Y, C))`

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1994
+  unsigned ShOpc = cast<Instruction>(ShiftedY)->getOpcode();
+  if (ShOpc != cast<Instruction>(ShiftedX)->getOpcode())
+    return nullptr;
----------------
This is probably not safe, it looks like `m_LogicalShift` also supports constant expressions.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2006
+  if (I.getOpcode() != Instruction::And && BWOpc != Instruction::And)
+      return nullptr;
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152568