[PATCH] D152927: [InstCombine] Fold binop op shifts with related amounts

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2125
+                                         IRBuilderBase &Builder) {
+  assert(I.isBitwiseLogicOp() && "Unexpected opcode");
+
----------------
https://alive2.llvm.org/ce/z/Y46Cnq `add` with `shl` works as well. Fine with only handling BW for now but maybe `InstructionCombining.cpp` is a better place to put this so its easier to extend in the future.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2136
+      !match(AddC,
+             m_SpecificInt_ICMP(ICmpInst::ICMP_ULT, APInt(BitWidth, BitWidth))))
+    return nullptr;
----------------
nit, since early out is an option, make this 2 seperate if statements to keep the expr a little less busy?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2146
+  // Both shifts must be the same.
+  Instruction::BinaryOps ShiftOp = (Instruction::BinaryOps)Op0Inst->getOpcode();
+  if (ShiftOp != Op1Inst->getOpcode())
----------------
static_cast instead of C-style


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2152
+      I.getOpcode(), ShiftedC1, Builder.CreateBinOp(ShiftOp, ShiftedC2, AddC));
+  return Builder.CreateBinOp(ShiftOp, NewC, ShAmt);
+}
----------------
nit: BinaryOperator::create(...) then drop the `replaceInstUsesWith` below?


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

https://reviews.llvm.org/D152927