[PATCH] D41953: [LoopUnroll] Unroll and Jam

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

> this looks like a rather complete implementation (as opposed to some WIP prototype)

Hopefully. The things I'm not super happy with are where this fits into the unroll pass, and the tests in test/Other. Whether this is a new pass or a part of unroll is a little awkward because of the way we need to do the outer unroll-and-jam before we do the inner unroll.

The DA Analysis pass either needs to be added to the set of preserved loop passes, or (because it's trivial to create and holds no state) we could just create the DependenceInfo as needed. This would just need an AA in loop unroll.

> In your description you mentioned something about testing. I was wondering if you can elaborate a little bit on this, if you had some more experiences with this pass since posting it.

So I've ran the normal set of regression tests / supertest / perennial / test-suite / eembc / geekbench / spec's / etc. All without issue. It has also run probably a few hundred thousand csmith seeds (which is pretty good at creating the kind of nested loops with odd memory accesses that unroll and jam looks for). Those are all with -always-unroll-and-jam to bypass the profitability check. (There can, of course, always be bugs)

> probably the other thing people first want to know if this is (always) beneficial for their back-end. Do you have some data/experience with this?

With the way DA and the profitability checks are right now, it may be a little conservative in when it is enabled. I have to look into this with the latest DA changes. I would expect it to be a win in most cases though. The performance is something I hope to work on more, so long as the basics are OK here. When it does fire, in the ideal case, it can help quite a bit with removing extra loads.



================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:267
 
+void llvm::simplifyLoopAfterUnroll(Loop *L, bool SimplifyIVs, LoopInfo *LI,
+                                   ScalarEvolution *SE, DominatorTree *DT,
----------------
SjoerdMeijer wrote:
> Looks like this is used by both LoopUnroll and UnrollAndJam. To simplify things, is it worth to separate this out in a different patch if this already used/beneficial by/for LoopUnroll?
Sure, sounds good. I have split out the DA and other unrelated parts. This one is less useful on it's own, if it's only used in one place, but I can split it out as a cleanup.


Repository:
  rL LLVM

https://reviews.llvm.org/D41953