[PATCH] D146350: [InstCombine] More aggressively try and fold irem/idiv/mul into selects.

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:191
 
+static std::optional<bool> shouldFoldOpIntoSelect(BinaryOperator &I, Value *Op,
+                                                  Value *OpOther) {
----------------
Comment that it returns MultiUse.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:193
+                                                  Value *OpOther) {
+  if (match(Op, m_Select(m_Value(), m_Value(), m_Value())))
+    return match(OpOther, m_ImmConstant()) &&
----------------
Matching against all-wildcards is a bit silly :)


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1020
+  const SimplifyQuery Q = SQ.getWithInstruction(&I);
+  if (isKnownNonZero(Op1, DL, 0, Q.AC, Q.CxtI, Q.DT)) {
+    if (auto MultiUse = shouldFoldOpIntoSelect(I, Op0, Op1))
----------------
Please use isSafeToSpeculativelyExecute() here. This check is not sufficient for sdiv (due to INT_MIN / -1), and we don't want to duplicate logic.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1847
+  const SimplifyQuery Q = SQ.getWithInstruction(&I);
+  if (isKnownNonZero(Op1, DL, 0, Q.AC, Q.CxtI, Q.DT)) {
+    if (auto MultiUse = shouldFoldOpIntoSelect(I, Op0, Op1))
----------------
Same here.


================
Comment at: llvm/test/Transforms/InstCombine/binop-select.ll:548
   ret i32 %div
 }
----------------
Also test the straightforward case where the mul has a constant operand? Or is that already handled somewhere else (and if so, can we remove it)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146350