[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,
----------------
paulwalker-arm wrote:
> pawosm01 wrote:
> > 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.
> Perhaps it's worth replacing the hardwired value with a command line option that defaults to 5?
Agreed. I've added one.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152281