[PATCH] D17442: [LPM] Remove the last worrisome split in the primary loop pass pipeline, allowing LICM and friends to always run over the outer loop after unrolling has a chance to remove the inner loop.

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

In http://reviews.llvm.org/D17442#357429, @mzolotukhin wrote:

> > LoopPassManager [ loop passes A -> LoopSimplifyCFG -> loop passes B ]
>
> > SimplifyCFG
>
> > 
>
> > does worse than
>
> > 
>
> > LoopPassManager [ loop passes A ]
>
> > SimplifyCFG
>
> >  LoopPassManager [ loop passes B ]
>
>
> Yeah, in my experiments we did have a lot of cases like you described. Basically, `LoopSimplifyCFG` in the first option wasn't able to cleanup the code enough for `loop passes B` to be effective. I also tried adding loop deletion/etc. but with no luck either.
>  That said, I didn't spend much time on it, so I don't have deep details. I think it's worth investigating further, especially since this is an active are now.


The question is, what should we do *now*.

Note that today, the simplify-cfg run comes before the thing that I think needs the most cleanup: unroll. So this change won't regress *any* cleanup taking place prior to loop unrolling. So if "unroll" is the "loop passes A" in the above example, it already doesn't work, and this patch doesn't make it worse.

Today, loop unrolling definitely creates invariant instructions that we then fail to hoist with LICM. This patch will fix that. Similarly, this patch will fix cases where we need to re-rotate an outer loop after unrolling an inner loop in order to unroll the outer loop.

So that's why I suspect the best path is to make this change, and then incrementally improve the loop simplification passes until they are sufficient to handle the other cases.

Thoughts?

In http://reviews.llvm.org/D17442#357515, @mzolotukhin wrote:

> Just to make it clear: I absolutely support any steps in this direction, and I'm just concerned that if we do it right now we might be hit by huge regressions. If testing proves me wrong, I'm totally happy with it:)
>
> As for the current situation - we might need a clean-up after loop-unswitch and loop-rotate. Also, the issue is that if any of this transformations fails due to lack of cleanup, it'll create a chain reaction: others will likely also fail.
>
> So, I think we need to test this change, and if there are major problems, try to address them first (probably, by developing LoopSimplifyCFG).


Cool, could you help with benchmarking here? As I said, I don't think I have any interesting benchmarks for this kind of loop heavy code to really have confidence in anything. =/ I mean, I can run the test suite, but I'm really not sure how much data that really gives anyone.


http://reviews.llvm.org/D17442