[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?
> > 
> >   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 now recall that it was to prevent re-casting to SCEVAddRecExpr multiple times (better type safety).

 -Hal

> 
> > 
> >   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