[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