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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:808
+    else if (Instruction::isBitwiseLogicOp(I.getOpcode()) &&
+             BinOpc == Instruction::And) {
+      // Pass
----------------
Handling add+shl should be safe here as well: https://alive2.llvm.org/ce/z/dWf_6D

I think you can extract a helper to check `Instruction::isBitwiseLogicOp(BinOpc) || ShOpc == Instruction::Shl` and use that everywhere that `isBitWiseLogicOp()` is used.

(I wanted to suggest just excluding the add+lshr combination upfront so we don't have to worry about it everywhere else, but it is valid for the `and` case above. If you're okay with losing that, we can just check this in IsValidBinOpc and not worry about it after that anymore.)


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:812
+      // Pass
+    }
+    // If both BinOp1 and BinOp2 are bitwise ops and:
----------------
goldstein.w.n wrote:
> nikic wrote:
> > Does this case trigger for anything other than non-splat vectors? If not, please remove it. (Generally, don't add non-splat vector support it it requires //any// additional complexity.)
> Why is that? I'd argue this is very minimal complexity and think it is an issue that a significant number of our combines only work for splats. Generally we mark combines that fail because of lacking splat support not as "*_fail", but "*_todo" which seems to imply we *want* to handle them. Leaving out these 4-lines of code (that also provide an early exit before more expensive checks) seems equivelent to choosing to leave a simple addressable TODO.
It's not a lot of code, but it does increase the verification space of the transform non-trivially, for a transform that already has some combinatorial explosion going on.

Anyway, I'm okay with keeping this, but I would suggest thinking about whether non-trivial non-splat vector handling is really a good use of implementation, review and long-term maintenance time, for the optimization benefit it provides.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152568