[PATCH] D152281: [Transforms][LICM] Add the ability to undo unprofitable reassociation

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2701
+  // First, we need to make sure we should do the transformation.
+  SmallVector<Use *> Changes;
+  for (BinaryOperator *Op = nullptr, *OpNext = nullptr,
----------------
As a final note, I would rewrite this loop into a worklist iteration, something along these lines:
```
SmallVector<BinaryOperator *> Worklist;
Worklist.push_back(VariantOp);
while (!Worklist.empty()) {
  BinaryOperator *BO = Worklist.pop_back_val();
  if (!BO->hasOneUse())
    return false;
  BinaryOperator *Op0, *Op1;
  if (match(BO, m_FAdd(m_BinOp(Op0), m_BinOp(Op1))) &&
      I->hasAllocReassoc()) {
    Worklist.push_back(Op0);
    Worklist.push_back(Op1);
    continue;
  }
  if (BO->getOpcode() != Instruction::FMul || !BO->hasAllocReassoc() ||
      L.isLoopInvariant(BO))
    return false;
  Use &U0 = BO->getOperandUse(0);
  Use &U1 = BO->getOperandUse(1);
   if (L.isLoopInvariant(U0))
     Changes.push_back(&U0);
   else if (L.isLoopInvariant(U1))
     Changes.push_back(&U1);
   else
     return false;
   if (Changes.size() > 5U) // A reasonable upper limit
     return false;
}
```
This has the benefit of not relying on specific structure for the fadds (is there something that guarantees that these form a linear chain?) and I think is also less confusing because the fadd and fmul handling is pretty cleanly separated.

I also wanted to ask where the number 5 for the cutoff comes from. Is that just chosen arbitrarily?


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2741
+    Instruction *Ins = dyn_cast<Instruction>(U->getUser());
+    assert(Ins);
+    IRBuilder<> Builder(Preheader->getTerminator());
----------------
dyn_cast + assert == cast


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152281