[PATCH] D41953: [LoopUnroll] Unroll and Jam

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

Hi Dave,

I think we have looked long enough at it now, and it's time to get some experience with it.  :)  
It's off by default, so I don't think there's any harm done here. Once it is in, it's easier to throw
more code at it, get more experience, and see if we further need to tweak or tune it; perhaps
we get feedback from others as well who are interested in using it.

Let's wait a few more days with committing to give people one more chance to object :-)

Inlined just a few more comments, mainly nits.

Cheers.



================
Comment at: lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp:84
+// Prefix of "llvm.loop.unroll." returns true if we have any unroll metadata.
+static bool HasAnyUnrollPragma(const Loop *L, StringRef Prefix) {
+  if (MDNode *LoopID = L->getLoopID()) {
----------------
This is *almost* identical to GetUnrollMetadata() in LoopUnroll.cpp. Is this something that could be shared somehow? Or would that be too inconvenient?


================
Comment at: lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp:229
+    // outer loop and can become shared.
+    unsigned numInvariant = 0;
+    for (BasicBlock *BB : SubLoop->getBlocks()) {
----------------
nit: numInvariant -> NumInvariant


================
Comment at: lib/Transforms/Utils/LoopUnrollAndJam.cpp:72
+  // Check that all blocks in ForeBlocks together dominate the subloop
+  // TODOD: This might ideally be done better with a dominator/postdominators.
+  BasicBlock *SubLoopPreHeader = SubLoop->getLoopPreheader();
----------------
nit: TODOD


================
Comment at: lib/Transforms/Utils/LoopUnrollAndJam.cpp:398
+          Value *OldValue = Phi.getIncomingValue(b);
+          if (Value *LastValue = LastValueMap[OldValue]) {
+            Phi.setIncomingValue(b, LastValue);
----------------
nit: don't need the brackets here


================
Comment at: lib/Transforms/Utils/LoopUnrollAndJam.cpp:622
+                              DependenceInfo &DI) {
+  // TODOD This needs to properly treat anything outside the outer loop as
+  //   loop-invariant in the dependency checks.
----------------
Nit: TODOD


================
Comment at: test/Transforms/LoopUnrollAndJam/unroll-and-jam.ll:9
+; CHECK: entry:
+; CHECK:   br i1 %or.cond, label %for.outer.preheader, label %for.end
+; CHECK: for.outer.preheader:
----------------
dmgreen wrote:
> SjoerdMeijer wrote:
> > Do we need all these checks? And with all the variable names still there, it looks like a fragile test to me. Same for the other functions below.
> I think as we are only running a single pass (not -O3 or an entire backend), it should be relatively stable.
> 
> I've updated this one to be auto generated and show the entire code output. The others are usually testing some small portion of the resulting function.
Ok, fair enough, agreed.


https://reviews.llvm.org/D41953