[PATCH] D141875: [InstCombine] Reorder (shl (add/sub (shl x, C0), y), C1) -> (add/sub (shl x, C0 + C1), (shl y, C1))

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:325
 
-/// If we have a shift-by-constant of a bitwise logic op that itself has a
-/// shift-by-constant operand with identical opcode, we may be able to convert
-/// that into 2 independent shifts followed by the logic op. This eliminates a
-/// a use of an intermediate value (reduces dependency chain).
+/// If we have a shift-by-constant of a bin op (bitwise logic op or add w/ shl)
+/// that itself has a shift-by-constant operand with identical opcode, we may be
----------------
or add -> or add/sub


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:328
+/// able to convert that into 2 independent shifts followed by the logic op.
+/// This eliminates a a use of an intermediate value (reduces dependency chain).
 static Instruction *foldShiftOfShiftedLogic(BinaryOperator &I,
----------------
typo (it was already there but disguised) - "a a"


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:329
+/// This eliminates a a use of an intermediate value (reduces dependency chain).
 static Instruction *foldShiftOfShiftedLogic(BinaryOperator &I,
                                             InstCombiner::BuilderTy &Builder) {
----------------
This function name is too specific now since we are not limited to logic ops. Not sure if there's a compact description for the set of possibilities. If not, just go with foldShiftOfShiftedBinOp().


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:359-362
     return match(V, m_BinOp(ShiftOpcode, m_Value(), m_Value())) &&
            match(V, m_OneUse(m_Shift(m_Value(X), m_Constant(C0)))) &&
            match(ConstantExpr::getAdd(C0, C1),
                  m_SpecificInt_ICMP(ICmpInst::ICMP_ULT, Threshold));
----------------
This was strangely matched, so I reduced it:
c2ab7e2abd834d4ef7b0f277

It shouldn't change anything functionally here, but you may need to update/rebase to patch cleanly.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:367
+  // is not so we need to reorder if we match operand(1).
+  bool Reorder = false;
+  if (matchFirstShift(BinInst->getOperand(0)))
----------------
"Reorder" doesn't seem like the right description. I'd use the more literal "FirstShiftIsOp1" or something like that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141875