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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:808
+    else if (Instruction::isBitwiseLogicOp(I.getOpcode()) &&
+             BinOpc == Instruction::And) {
+      // Pass
----------------
nikic wrote:
> 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.)
> 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.
> 
Good catch (when I was playing around with combinations, I was using lshr so missed most of the add + shl cases).

> (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.)

I added `IsCompletelyDistributable` and early out if its not met after this check so the following two checks can ignore it.



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:812
+      // Pass
+    }
+    // If both BinOp1 and BinOp2 are bitwise ops and:
----------------
nikic wrote:
> 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.
> 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.

I guess this (`add` + `lshr`) highlight a difference in philosophy between us (that has come up on other reviews). My general feeling is the more that can be covered, the better. Its a lot more painful to have to rewrite code because the compiler isn't getting your case and updating the compiler doesn't reap rewards for most until at least a year or so down the line. More cases does imply code complexity, but generally think a clean 10k LOC is clearer than 1k messy LOC i.e, as long as coding standards are kept high and the niche cases don't degrade the quality of the file, they are worth it.
Maybe I'm trivializing some aspect of it however.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152568