[PATCH] D9151: Loop Versioning for LICM
Charlie Turner via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 27 15:53:10 PDT 2015
chatur01 requested changes to this revision.
chatur01 added a reviewer: chatur01.
chatur01 added a comment.
This revision now requires changes to proceed.
Hi Ashutosh,
This needs some superficial changes before it could go further. Some of which I've pointed out below. I'd suggest going through all the comments as well and checking grammar, I didn't point out all of that.
I see you have tested when your heuristics would be suitable to apply the transformation are triggered, but there's no tests for the transform itself. Would it not make sense to add some of those too?
I'm merely an interested observer here, I don't have the experience to be able to OK the implementation. Hopefully by sticking my oar in it will help get the attention of people that can :)
Regards,
Charlie.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:102
@@ +101,3 @@
+
+/// Loop Versioning invariant threshold
+static cl::opt<unsigned>
----------------
I think it would be more useful to describe what the threshold is for than to state the description twice. Seems redundant. Same goes for the other options.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:200
@@ +199,3 @@
+/// \brief Collects stride access from a given value.
+void LoopVersioningLICM::collectStridedAccess(Value *MemAccess) {
+ Value *Ptr = nullptr;
----------------
Why is this duplicated from LoopVectorizationLegality?
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:218
@@ +217,3 @@
+
+/// \brief Check loop structure and confirms its good for Loop Versioning.
+bool LoopVersioningLICM::loopStructureLegality() {
----------------
s/its/it's/
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:219
@@ +218,3 @@
+/// \brief Check loop structure and confirms its good for Loop Versioning.
+bool LoopVersioningLICM::loopStructureLegality() {
+ // Loop must have a preheader, if not return false.
----------------
That's a weird name for the function, do you think legalLoopStructure would be better? Thinking how it would read in a conditional.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:225
@@ +224,3 @@
+ }
+ // Loop should be innermost loops, if not return false.
+ if (CurLoop->getSubLoops().size()) {
----------------
s/loops/loop/
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:271
@@ +270,3 @@
+bool LoopVersioningLICM::loopMemoryLegality() {
+ // LoopAccessInfo should not be null.
+ assert(LAI != nullptr && "LoopAccessInfo should not be null");
----------------
Don't bother duplicating the assert reason in the line right above the assert.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:279-281
@@ +278,5 @@
+ }
+ bool HasMayAlias = 0;
+ bool TypeSafety = 0;
+ bool IsMod = 0;
+ SmallPtrSet<Value *, 16> PointerSet;
----------------
Use = false here
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:285
@@ +284,3 @@
+ // Iterate over alias set tracker and individual alias sets.
+ // 1) Make sure PointerSet should have atleast 2 pointers.
+ // 2) PointerSet should'nt have pointers more then RuntimeMemoryCheckThreshold
----------------
Sloppy sentence. Put a space between at and least
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:286
@@ +285,3 @@
+ // 1) Make sure PointerSet should have atleast 2 pointers.
+ // 2) PointerSet should'nt have pointers more then RuntimeMemoryCheckThreshold
+ // 3) Make sure AliasSets should not have any must alias set.
----------------
shouldn't
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:287
@@ +286,3 @@
+ // 2) PointerSet should'nt have pointers more then RuntimeMemoryCheckThreshold
+ // 3) Make sure AliasSets should not have any must alias set.
+ for (AliasSetTracker::iterator I = CurAST->begin(), E = CurAST->end(); I != E;
----------------
Make sure the alias set doesn't have any MustAlias set.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:288-289
@@ +287,4 @@
+ // 3) Make sure AliasSets should not have any must alias set.
+ for (AliasSetTracker::iterator I = CurAST->begin(), E = CurAST->end(); I != E;
+ ++I) {
+ AliasSet &AS = *I;
----------------
Could you instead use the fancy new range-for?
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:291
@@ +290,3 @@
+ AliasSet &AS = *I;
+ // Skip Forward Alias Sets, dont consider ignored alias sets object.
+ if (AS.isForwardingAliasSet())
----------------
I don't find this comment useful, I'd be more interested in /why/ we skip forward alias sets, I can see from the code that we do. If it's completely obvious why we skip forward alias sets, then the comment isn't needed.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:294
@@ +293,3 @@
+ continue;
+ // Return false in case of MustAlias
+ if (AS.isMustAlias())
----------------
Same here.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:298
@@ +297,3 @@
+ Value *SomePtr = AS.begin()->getValue();
+ bool typeCheck = 1;
+ // Check for Ismod & MayAlias
----------------
Coding violation
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:299
@@ +298,3 @@
+ bool typeCheck = 1;
+ // Check for Ismod & MayAlias
+ HasMayAlias |= AS.isMayAlias();
----------------
Try to maintain some consistency in your spelling of these things.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:356-359
@@ +355,6 @@
+ CallInst *CI = dyn_cast<CallInst>(I);
+ if (CI && !getIntrinsicIDForCall(CI, TLI) && !isa<DbgInfoIntrinsic>(CI) &&
+ !(CI->getCalledFunction() && TLI &&
+ TLI->isFunctionVectorizable(CI->getCalledFunction()->getName()))) {
+ DEBUG(dbgs() << " Found a non-intrinsic, non-libfunc callsite.\n");
+ return false;
----------------
Ooof, that conditional is pretty hairy :)
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:374
@@ +373,3 @@
+ }
+ LoadnStoreCounter++;
+ collectStridedAccess(Ld);
----------------
LoadAndStoreCounter
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:404-405
@@ +403,4 @@
+ // Resetting counters.
+ LoadnStoreCounter = 0;
+ InvariantCounter = 0;
+ isReadOnlyLoop = true;
----------------
Use = false
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:406
@@ +405,3 @@
+ InvariantCounter = 0;
+ isReadOnlyLoop = true;
+ // Instruction check: Iterate over loop blocks and instructions of each block
----------------
Coding convention
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:409-410
@@ +408,4 @@
+ // and check instruction safety.
+ for (Loop::block_iterator bb = CurLoop->block_begin(),
+ be = CurLoop->block_end();
+ bb != be; ++bb) {
----------------
Coding convetion and also range for
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:412-413
@@ +411,4 @@
+ bb != be; ++bb) {
+ for (BasicBlock::iterator it = (*bb)->begin(), e = (*bb)->end(); it != e;
+ ++it) {
+ // If instruction in unsafe just return false.
----------------
and again
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:437-438
@@ +436,4 @@
+ // Check invariant threshold, should be in limit.
+ if ((((float)(InvariantCounter) / (float)(LoadnStoreCounter)) * 100) <
+ InvariantThreshold) {
+ DEBUG(dbgs()
----------------
You don't need all those parens. (float)a / float(b) * 100. < c should be fine. Put it in a variable so you don't repeat it below as well.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:512
@@ +511,3 @@
+ MDNode *NewDomain = MDB.createAnonymousAliasScopeDomain("LVDomain");
+ std::string Name = "LVAliasScope";
+ SmallVector<Metadata *, 4> Scopes, NoAliases;
----------------
StringRef
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:517-521
@@ +516,7 @@
+ // set no-alias for all load & store instructions.
+ for (Loop::block_iterator bb = VerLoop->block_begin(),
+ be = VerLoop->block_end();
+ bb != be; ++bb) {
+ for (BasicBlock::iterator it = (*bb)->begin(), e = (*bb)->end(); it != e;
+ ++it) {
+ // Only interested in instruction that may modify or read memory.
----------------
Range for, coding convention!
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:563-564
@@ +562,4 @@
+ // Loop over the body of this loop, construct AST.
+ for (Loop::block_iterator I = L->block_begin(), E = L->block_end(); I != E;
+ ++I) {
+ BasicBlock *BB = *I;
----------------
Range for
Repository:
rL LLVM
http://reviews.llvm.org/D9151
More information about the llvm-commits
mailing list