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

[Paul Osmialowski via Phabricator via llvm-commits]

pawosm01 added a comment.





================
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,
----------------
pawosm01 wrote:
> 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.
> 
> I also wanted to ask where the number 5 for the cutoff comes from. Is that just chosen arbitrarily?

This was chosen arbitrarily indeed, the choice was based on an observation of the problem at hand.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152281