[PATCH] Loop Rerolling Pass

[Hal Finkel]

----- Original Message -----
> ----- Original Message -----
> > 
> >   Why are you looking for Phi-IVs across all loop blocks instead of
> >   just the header block in collectPossibleIVs and
> >   collectPossibleReductions?

I had some thought that, in general, the PHIs could be in other blocks in the loop, not just the header. However, unless we had some chain of BBs linked with unconditional branches, this likely can't be the case (and still have the PHI be useful). I'll just search the header.

 -Hal

> > 
> >   Why is collectPossibleReductions a separate pass from
> >   collectPossibleIVs?
> 
> These could be merged. I split them in order to make the code easier
> to read, and so that we did not bother analyzing possible reductions
> if no suitable IVs for rerolling are available.
> 
> > 
> >   I'm not sure what IVInfo is for. It looks like the SCEV field is
> >   always the result of calling getSCEV on the first field.
> 
> I had thought that it would be better to cache the result so I would
> not need to call getSCEV again. As you imply, this is probably not
> worthwhile.
> 
> > 
> >   I'm not sure why findScaleFromMul needs to look for Mul
> >   instructions instead of examining SCEVs.
> 
> I probably should look at the SCEVs (because the Mul might be a
> shift, etc.).
> 
> > 
> >   I don't know what "Final" is for.
> 
> When collecting the set of users, there are three inputs:
>  - The root instruction
>  - The set of excluded instructions (which are never added to the use
>  set even if they are users)
>  - The set of final instructions (which are added to the use set if
>  they are users, but on which we don't recurse looking for more
>  users)
> 
> > 
> >   Eventually I had to punt on the review. I wasn't able to grok
> >   everything that is going on in LoopReroll::reroll. The function
> >   is
> >   > 400 lines, with lots of state. Since I don't really understand
> >   the algorithm, it's hard for me to suggest a better way of doing
> >   it. If the results are good and compilation time isn't terrible,
> >   I
> >   can't argue against checking it in as a prototype. It needs a lot
> >   of improvement though before I would consider it
> >   reviewable/maintainable.
> 
> I don't disagree with you. It still looks like something I hacked
> together at midnight, and I'm still actively working on refactoring,
> adding more comments, etc. The question is just whether I can do
> this in-tree or whether more is needed before an initial checkin.
> 
> > 
> > 
> > ================
> > Comment at: lib/Transforms/Scalar/LoopRerollPass.cpp:215
> > @@ +214,3 @@
> > +        if (const SCEVConstant *IncSCEV =
> > +            dyn_cast<SCEVConstant>(PHISCEV->getOperand(1))) {
> > +          if
> > (!IncSCEV->getValue()->getValue().isStrictlyPositive())
> > ----------------
> > Did you mean PHISCEV->getStepRecurrence()
> 
> Yes.
> 
> Thanks again,
> Hal
> 
> > 
> > 
> > http://llvm-reviews.chandlerc.com/D1940
> > 
> 
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory