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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n marked 6 inline comments as done.
goldstein.w.n added a comment.

In D152568#4412078 <https://reviews.llvm.org/D152568#4412078>, @nikic wrote:

> It looks like there are some more tests that need to be updated.

Fixed.



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152568