[PATCH] D38037: [InstCombine] Compacting or instructions whose operands are shift instructions

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2237
+    // match (V&C1)<<C2
+    static auto MatchShiftOfAnd =
+        [](Value *V, Value *&Source, Instruction::BinaryOps &ShiftOpcode,
----------------
Shift of Ands are canonicalized to And of Shifts unless the 'and' has an additional use. I don't think your test cases cover that.


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2255
+    // match (V<<C1)<<C1
+    static auto MatchAndOfShift =
+        [](Value *V, Value *&Source, Instruction::BinaryOps &ShiftOpcode,
----------------
I don't think there's any reason to make these lambda's "static"


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2305
+            Builder.CreateBinOp(ShiftOpcode0, MaskedSource, ShiftBy0);
+        Op0->replaceAllUsesWith(NewOp0);
+        Value *NewOp1 =
----------------
Why the replaceAllUsesWith call? We don't normally do this in InstCombine. Are you trying to replace other uses than this OR? Do you have test cases for that?


Repository:
  rL LLVM

https://reviews.llvm.org/D38037