[PATCH] [Patch] Loop Interchange Pass

hfinkel at anl.gov hfinkel at anl.gov
Mon Feb 23 13:31:08 PST 2015


> I had some time to work on generic version of loop interchange to support any depth. This updated patch supports loops of any depth.


Nice! A few comments...


================
Comment at: include/llvm/Transforms/Scalar.h:137
@@ +136,3 @@
+//
+// LoopInterchange - This pass is interchanges loops to give better cache hits.
+//
----------------
You've un-improved this comment; please change it back.

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:262
@@ -261,3 +261,3 @@
   addExtensionsToPM(EP_ScalarOptimizerLate, MPM);
-
+  MPM.add(createLoopInterchangePass());
   if (RerollLoops)
----------------
I'd certainly like to have this on by default eventually, but we should be more conservative at first. Please add a command-line flag to enable this (there are several in this file already), so we can do further testing.

================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:11
@@ +10,3 @@
+// This Pass handles loop interchange transform. This interchanges the inner
+// and outer loop if interchanging can result in better cache hits or fine
+// grained parallelism.
----------------
You should use the 'cache-friendly memory access pattern' terminology here too.

================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:50
@@ +49,3 @@
+typedef SmallVector<Loop *, 8> LoopVector;
+typedef std::vector<std::vector<char>> CharMatrix;
+class LoopInterchange;
----------------
Is this matrix generally sparse? (or could we make it sparse by picking some default). If so, is this the right data structure?

================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:74
@@ +73,3 @@
+
+bool populateDependencyMatrix(CharMatrix &DepMatrix, unsigned Level, Loop *L,
+                              DependenceAnalysis *DA) {
----------------
I'm somewhat worried about doing this eagerly for all loops; what if they're really large with lots of memory accesses? Maybe we should have a cutoff?


================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:652
@@ +651,3 @@
+    PhiCount++;
+  }
+  return PhiCount;
----------------
No need for { } here.

================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:855
@@ +854,3 @@
+  // TODO: Improve this heuristic to catch more cases.
+  // If the inner loop is loop independent or doesn't carry any dependency it is
+  // profitable to move this to outer position.
----------------
I don't really understand this comment. I think we can assume that LICM has run first. (and if this pass detects loop-invariant code better than LICM, that is another problem to fix, but not here).

================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:916
@@ +915,3 @@
+  for (Loop::iterator I = InnerLoop->begin(), E = InnerLoop->end(); I != E;
+       ++I) {
+    OuterLoop->addChildLoop(InnerLoop->removeChildLoop(I));
----------------
No need for the { }

http://reviews.llvm.org/D7499

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list