[PATCH] D41953: [LoopUnroll] Unroll and Jam

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Hi Dave, thanks for making this into a pass, I think this looks a lot better now. I skimmed through the whole diff for first time, and just wrote down some things I noticed, mostly nitpicks, see the comments inlined.  I will now study the different bits and pieces in more detail.



================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:427
+    bool UnrollAndJam;
+    /// Threshold for unroll and jam, for inner loop size
+    unsigned UnrollAndJamInnerLoopThreshold;
----------------
Can you be a bit more explicit here what the threshold exactly is?


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:427
+    bool UnrollAndJam;
+    /// Threshold for unroll and jam, for inner loop size
+    unsigned UnrollAndJamInnerLoopThreshold;
----------------
SjoerdMeijer wrote:
> Can you be a bit more explicit here what the threshold exactly is?
typo: trashold :)


================
Comment at: lib/Analysis/DependenceAnalysis.cpp:110
 static cl::opt<bool>
-Delinearize("da-delinearize", cl::init(false), cl::Hidden, cl::ZeroOrMore,
+Delinearize("da-delinearize", cl::init(true), cl::Hidden, cl::ZeroOrMore,
             cl::desc("Try to delinearize array references."));
----------------
Will other people get unhappy when we flip this switch?


================
Comment at: lib/Target/ARM/ARMTargetTransformInfo.cpp:626
+  UP.UnrollAndJam = true;
+  UP.UnrollAndJamInnerLoopThreshold = 60;
 
----------------
Some comments perhaps why it is set to 60?


================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:659
+    if (EnableUnrollAndJam) {
+      // Unroll and Jam. Before unroll but in a different LPM
+      MPM.add(createLoopUnrollAndJamPass(OptLevel));
----------------
Elaborate a bit what you mean here (the 2nd sentence)?


================
Comment at: lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp:148
+    unsigned InnerLoopSize, TargetTransformInfo::UnrollingPreferences &UP) {
+  // Check for explicit Count from the  "unroll-and-jam-count" option.
+  bool UserUnrollCount = UnrollAndJamCount.getNumOccurrences() > 0;
----------------
nit: extra space


================
Comment at: lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp:203
+
+  // If the inner loop count is know and small, leave the entire loop nest to
+  // be the unroller
----------------
nit: know -> known


================
Comment at: lib/Transforms/Scalar/LoopUnrollAndJamPass.cpp:305
+  }
+  if (NumInlineCandidates != 0) {
+    DEBUG(dbgs() << "  Not unrolling loop with inlinable calls.\n");
----------------
nit: don't need the "!= 0"


================
Comment at: lib/Transforms/Utils/LoopUnrollAndJam.cpp:121
+
+/*
+  This method performs Unroll and Jam. For a simple loop like:
----------------
Some more nitpicking here, about names.

Perhaps "Fore" can be confusing: is it another loop, a block? So something like this:

  for (i)
    block[i, 0]
    for (j)
      block[j, 0]
    end
    block[i, 1]
  end
      
becomes:

  for (i, i+=2)
    block[i, 0]
    block[i+1, 0]
    for (j, j+=4)
      block[j+0, 0]
      block[j+1, 0]
      block[j+2, 0]
      block[j+3, 0]
    end
    block[i, 1]
    block[i+1, 1]
  end

Mentioning that the outer loop has been unrolled with a factor of 2, and the inner loop with an unroll factor of 4.

Not sure if the classic loopunroll and jam literature uses standard terminology here, but perhaps inner/outer loop rather than "Fore, Subloop and Aft".

Also, perhaps you want to mention some restrictions somewhere, if there are any. E.g., can this doubly nested loop occur in more deeply nested loop structure (and loopunroll and jam still trigger)?


================
Comment at: lib/Transforms/Utils/LoopUnrollAndJam.cpp:139
+
+  So the blocks are essentially outer unrolled and then fused ("jammed")
+  together into a single inner loop. This can increase speed when there are
----------------
Blocks = outer loops?

So perhaps something along the lines of:

"So the outer loops are unrolled and the inner loops fused ("jammed")."


================
Comment at: lib/Transforms/Utils/LoopUnrollAndJam.cpp:180
+                                    UnrollRemainder, LI, SE, DT, AC, true)) {
+      DEBUG(dbgs() << "Wont unroll-and-jam; remainder loop could not be "
+                      "generated when assuming runtime trip count\n");
----------------
Nit: wont


================
Comment at: lib/Transforms/Utils/LoopUnrollAndJam.cpp:196
+  if (CompletelyUnroll) {
+    DEBUG(dbgs() << "COMPLETELY UNROLL AND JAMMING loop %" << Header->getName()
+                 << " with trip count " << TripCount << "!\n");
----------------
nit: don't need to capitalize this message?


================
Comment at: lib/Transforms/Utils/LoopUnrollAndJam.cpp:210
+
+    DEBUG(dbgs() << "UNROLL AND JAMMING loop %" << Header->getName() << " by "
+                 << Count);
----------------
same here?


================
Comment at: lib/Transforms/Utils/LoopUnrollAndJam.cpp:542
+
+  // We shouldnt have done anything to break loop simplify form or LCSSA.
+  Loop *OutestLoop = OuterL ? OuterL : (!CompletelyUnroll ? L : SubLoop);
----------------
typo: shouldnt


https://reviews.llvm.org/D41953