[PATCH] D9151: Loop Versioning for LICM

Hal Finkel via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 15 11:18:26 PST 2016


hfinkel added a comment.

I think this is getting really close to being ready.


================
Comment at: docs/LangRef.rst:4531
@@ -4530,1 +4530,3 @@
 
+'``llvm.loop.licm_versioning``' Metadata
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
----------------
For consistency with our other metadata (such as llvm.loop.unroll.disable), we should name this:

  llvm.loop.licm_versioning.disable


================
Comment at: docs/LangRef.rst:4534
@@ +4533,3 @@
+
+This metadata indicates that the loop is already versioned by loop versioning
+for loop invariant code motion (LICM). This metadata is applicable on both the 
----------------
This paragraph is too implementation-specific. How about this:

  This metadata indicates that the loop should not be versioned for the purpose of enabling loop-invariant code motion (LICM). The metadata has a single operand which is the string `llvm.loop.licm_versioning.disable`.

================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:380
@@ +379,3 @@
+/// else return false.
+bool checkStringMetadataIntoLoop(Loop * TheLoop, StringRef Name);
+
----------------
Remove space in between the * and `TheLoop`.

================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:383
@@ +382,3 @@
+/// \brief Set input string into loop metadata by keeping other values intact.
+void writeStringMetadataIntoLoop(Loop *TheLoop, const char *MDString, 
+                                 unsigned V = 0);
----------------
How about calling this:

  addStringMetadataToLoop

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:10
@@ +9,3 @@
+//
+// Most of the time when alias analysis is uncertain about memory access and it
+// assumes may-alias. This uncertainty from alias analysis restricts LICM from
----------------
This first statement is actually misleading; it should always return MayAlias when it is uncertain. How about saying this:

  Then alias analysis is uncertain about the aliasing between any two accesses, it will return MayAlias.

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:12
@@ +11,3 @@
+// assumes may-alias. This uncertainty from alias analysis restricts LICM from
+// proceeding further. In cases where alias analysis is uncertain we will use
+// loop versioning as an alternative.
----------------
will -> might

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:16
@@ +15,3 @@
+// Loop Versioning will create a version of the loop with aggressive aliasing
+// assumptions and the other with conservative (default) aliasing assumptions.
+// The version of the loop making aggressive aliasing assumptions will have all
----------------
and the other -> in addition to the original

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:21
@@ +20,3 @@
+// checks for all unique memory accessed in loop, and it ensures the lack of
+// memory aliasing. Based on this check result at runtime any of the loop gets
+// executed, If the runtime check detects any memory aliasing, then the original
----------------
"Based on this check result at runtime any of the loop gets executed," -> The result of the runtime check determines which of the loop versions is executed:

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:28
@@ +27,3 @@
+//
+// a) Perform LoopVersioningLICM feasibility check.
+// b) If loop is a candidate for versioning then create a memory bound check,
----------------
LoopVersioningLICM -> LoopVersioningLICM's

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:30
@@ +29,3 @@
+// b) If loop is a candidate for versioning then create a memory bound check,
+//    by considering all the memory access in loop body.
+// c) Clone original loop and set all memory access as no-alias in new loop.
----------------
access -> accesses

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:31
@@ +30,3 @@
+//    by considering all the memory access in loop body.
+// c) Clone original loop and set all memory access as no-alias in new loop.
+// d) Set original loop & versioned loop as a branch target of runtime check
----------------
access -> accesses

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:32
@@ +31,3 @@
+// c) Clone original loop and set all memory access as no-alias in new loop.
+// d) Set original loop & versioned loop as a branch target of runtime check
+//    result.
----------------
of runtime check -> of the runtime check

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:35
@@ +34,3 @@
+//
+// It transform loop as shown below:
+//
----------------
transform -> transforms

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:95
@@ +94,3 @@
+#define DEBUG_TYPE "loop-versioning-licm"
+#define LOOP_VERSIONING_LICM_METADATA "llvm.loop.licm_versioning"
+
----------------
Please update to llvm.loop.licm_versioning.disable

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:100
@@ +99,3 @@
+/// Threshold minimum allowed percentage for possible
+/// invariant instruction in a loop.
+static cl::opt<float>
----------------
instruction -> instructions

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:103
@@ +102,3 @@
+    LVInvarThreshold("-licm-versioning-invariant-threshold",
+                     cl::desc("LoopVersioningLICM threshold minimum "
+                              "allowed percentage for possible "
----------------
LoopVersioningLICM threshold minimum -> LoopVersioningLICM's minimum

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:104
@@ +103,3 @@
+                     cl::desc("LoopVersioningLICM threshold minimum "
+                              "allowed percentage for possible "
+                              "invariant instruction in a loop"),
----------------
for -> of

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:105
@@ +104,3 @@
+                              "allowed percentage for possible "
+                              "invariant instruction in a loop"),
+                     cl::init(25), cl::Hidden);
----------------
instruction -> instructions

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:105
@@ +104,3 @@
+                              "allowed percentage for possible "
+                              "invariant instruction in a loop"),
+                     cl::init(25), cl::Hidden);
----------------
hfinkel wrote:
> instruction -> instructions
in a -> per

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:112
@@ +111,3 @@
+    cl::desc(
+        "LoopVersioningLICM threshold for maximum allowed loop nest/depth"),
+    cl::init(2), cl::Hidden);
----------------
LoopVersioningLICM -> LoopVersioningLICM's

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:118
@@ +117,3 @@
+    LVMemchkPtrThreshold("loopver-memcheck-ptr-threshold",
+                         cl::desc("LoopVersioningLICM threshold for maximum "
+                                  "possible pointers allowed in memcheck"),
----------------
LoopVersioningLICM's maximum number of pointers per runtime check

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:302
@@ +301,3 @@
+  // Loop should have a trip count, if not return false.
+  const SCEV *ExitCount = SE->getBackedgeTakenCount(CurLoop);
+  if (ExitCount == SE->getCouldNotCompute()) {
----------------
Why are you checking this?

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:386
@@ +385,3 @@
+  // Check memory threshold, should be in limit.
+  if (PointerSet.size() > RuntimeMemoryCheckThreshold) {
+    DEBUG(dbgs() << "    Loop body has pointers more then defined threshold\n");
----------------
Shouldn't you use LAI->getNumRuntimePointerChecks() here instead of PointerSet.size()?

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:413
@@ +412,3 @@
+  // Hence we don't need to version anything in this case.
+  if (CurLoop->isAnnotatedParallel()) {
+    DEBUG(dbgs() << "    Parallel loop is not worth versioning\n");
----------------
Performing this check per instruction seems silly. Maybe move to legalLoopStructure?

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:632
@@ +631,3 @@
+char LoopVersioningLICM::ID = 0;
+INITIALIZE_PASS_BEGIN(LoopVersioningLICM, "do-loop-versioning-licm",
+                      "Loop Versioning For LICM", false, false)
----------------
do-loop-versioning-licm -> loop-versioning-licm

(I often just use DEBUG_TYPE for this; there's no reason for them to be out-of-sync)

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:643
@@ +642,3 @@
+INITIALIZE_PASS_DEPENDENCY(TargetLibraryInfoWrapperPass)
+INITIALIZE_PASS_END(LoopVersioningLICM, "do-loop-versioning-licm",
+                    "Loop Versioning For LICM", false, false)
----------------
do-loop-versioning-licm -> loop-versioning-licm (or DEBUG_TYPE)


Repository:
  rL LLVM

http://reviews.llvm.org/D9151





More information about the llvm-commits mailing list