[PATCH] Loop Rerolling Pass

[Hal Finkel]

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