[PATCH] D77523: Add CanonicalizeFreezeInLoops pass

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D77523#2014839 <https://reviews.llvm.org/D77523#2014839>, @jdoerfert wrote:

> In D77523#2014624 <https://reviews.llvm.org/D77523#2014624>, @aqjune wrote:
>
> > Hi,
> >
> > In D77523#2012741 <https://reviews.llvm.org/D77523#2012741>, @jdoerfert wrote:
> >
> > > I'm asking myself if it wouldn't be better to start with the freeze instructions and work our way up through the operands. At the end of the day we do a lot of work just to figure out there is no freeze or the instruction chains we build do not end in one. Am I missing something?
> >
> >
> > It's true, but we should iterate over instructions inside the loop to find freeze instructions.
> >  Indexing freezes that are uses of induction variables will be helpful for exiting early; right now, DivRemPair is the place where it introduces freezes, but it does not do relevant conversion in x86-64, so the code will have almost zero freeze instructions.
> >  Would it make sense if IVUsers maintains the info (existence of freeze)? I'm not familiar with the class, but may have a look.
>
>
> It might but that means maintaining state, which is hard and error prone.
>
> I was thinking more along the lines of this:
>
>   for (auto *I : L->instruction())
>     if (auto *Fr = dyn_cast<Freeze>(I))
>       handleFreeze(Fr);
>  
>   handleFreeze(L, Fr) { 
>   Worklist = {Fr};
>   while (!Worklist.empty()) {
>     Inst =  dyn_cast<Instruction>(Worklist.pop())
>     if (!Inst || !L->contains(Inst) || !Visited.insert(V))
>       continue;
>     if (isSpecialPHI(V))
>       // found special PHI that ends in a freeze, do record it
>     for (auto &Op : Inst->operands())
>       Worklist.push_back(Op);
>   }
>   }
>
>
> The main difference is that we are performing a single traversal of the loop + work per freeze instruction that is bounded by the def-use chain ending in the freeze and contained in the loop.


I guess it depends on what kinds of freeze instructions we are looking for exactly.

IIRC the earlier version of the patch only considered freeze instructions where the operand is either an induction PHI or the step instruction. If we only look for those cases, checking only the users of the PHI and the step instruction potentially avoids having to iterate over large loop bodies (also, parent loops contain all blocks of their child loops, so I think iterating over all instructions in a loop would mean revisiting all instructions in child loops).

If we are looking for any freeze that is reachable from an induction PHI, then the benefit of starting at a PHI will probably much less.

But I thought the problem that this pass is supposed to solve is quite narrow: if there is a freeze in a cycle involving an induction PHI, push the freeze out of the loop. To detect those cases, it should be enough to (recursively) check the operands of the step instruction. That should only require a few steps at most, as the instructions that can be part of such a cycle are quite limited.

FWIW I think ideally the patch should be kept as narrow as possible to handle the motivating case initially and additional cases as follow-ups as required. That makes the reviews easier, reduces risk, lowers the threshold for enabling it and allows to better analyse the impact of extensions on its own. For example, there might not be much (any?) benefit of pushing certain freezes out of a loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77523