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

[Paul Osmialowski via Phabricator via llvm-commits]

pawosm01 marked an inline comment as done.
pawosm01 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,
----------------
nikic wrote:
> 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?
Well, I deliberately wanted to avoid recursion, even if explicitly using stack (here named a Worklist). But this looks so neat that let's have it this way.



================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2741
+    Instruction *Ins = dyn_cast<Instruction>(U->getUser());
+    assert(Ins);
+    IRBuilder<> Builder(Preheader->getTerminator());
----------------
nikic wrote:
> dyn_cast + assert == cast
ah, I didn't notice that, I'll get back to it soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152281