[PATCH] Loop Versioning for LICM
Nema, Ashutosh
Ashutosh.Nema at amd.com
Fri Jun 19 02:29:58 PDT 2015
Thanks Philip for this review.
Drive by review. A few comments here or there.
Meta comments:
- Code structure and naming is somewhat confusing. Attention to factoring of functions and method names would help.
- This feels like too large a patch to easily review. I would strongly prefer that you split the patch into the smallest possible piece (i.e. it works on trivial cases only), then extend. Given Hal has already started reviewing this, I'll defer to his preferences here, but I'm unlikely to signoff on a patch this large and complex.
I'll try to divide this patch at least at logical level.
Also as suggested there are possibly few functions that can be moved as utility.
I'll come-up with a separate patch for them.
REPOSITORY
rL LLVM
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:3
@@ +2,3 @@
+//
+// The LLVM Compiler Infrastructure
+//
----------------
Please follow the standard header format. See LICM as an example.
It would help to have a trivial example here. The shortest possible while loop which shows the transform would make the discussion much more clear.
Sure will update it and add an example.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:57
@@ +56,3 @@
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/Analysis/ScalarEvolutionExpander.h"
+#include "llvm/Transforms/Utils/BasicBlockUtils.h"
----------------
Include order should be sorted.
Sure will sort the included headers and update.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:78
@@ +77,3 @@
+
+/// Loop Versioning invariant threshold
+static cl::opt<unsigned>
----------------
What is an "invariant threshold"? You probably need a better name for this.
This option is to control the profitability of loop versioning for LICM.
It checks invariant load & store vs total instruction of loop. If invariants
are more than defined threshold then only go for LICM loop versioning.
Threshold is defined in percentage with a default value 25%.
How about "-licm-versioning-threshold" or "-licm-versioning-invariant-threshold" ?
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:124
@@ +123,3 @@
+/// \brief Recursively clone the specified loop and all of its children,
+/// mapping the blocks with the specified map.
+static Loop *cloneLoop(Loop *L, Loop *PL, ValueToValueMapTy &VM, LoopInfo *LI,
----------------
I believe this is duplicated code with LICM. Please fine a place to put a utility function instead. Transforms/Utils/Loops.h would be fine.
Ideally, this would be split into it's own patch.
Sure I'll move this to a utility.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:145
@@ +144,3 @@
+/// return the induction operand of the gep pointer.
+static Value *stripGetElementPtr(Value *Ptr, ScalarEvolution *SE, Loop *Lp) {
+ GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(Ptr);
----------------
A better name would help here.
Don't we have similar code in CGP?
Similar function exists in LoopVectorizer will change it as a utility and use it.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:161
@@ +160,3 @@
+
+/// \brief Look for a cast use of the passed value.
+static Value *getUniqueCastUse(Value *Ptr, Loop *Lp, Type *Ty) {
----------------
Er, what does this actually do? The Ty and L params make me thing this isn't about finding a unique use...
Similar function exists in LoopVectorizer will change it as a utility and use it.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:686
@@ +685,3 @@
+/// +----------+
+Loop *LoopVersioningLICM::versionizeLoop(LPPassManager &LPM) {
+ std::vector<BasicBlock *> LoopBlocks;
----------------
I suspect there is code which can and should be commoned with unswitch here.
I'll check and come back to you.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:826
@@ +825,3 @@
+ // Only interested in load & stores
+ if (!it->mayReadFromMemory() && !it->mayWriteToMemory())
+ continue;
----------------
Your comment and code disagree. Are you intentionally handling RMW operations?
Will update comments here, we are handling RMW operations.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:833
@@ +832,3 @@
+ NoAliases.push_back(NewScope);
+ // Set no-alias for current instruction.
+ I->setMetadata(
----------------
The loop structure is very odd here. It looks like later instructions end up with more noalias markers than earlier ones? I suspect that's not what you want.
Here we want to make no-alias to instructions, yes the last one will have more no-alias.
But its required to make instruction no-alias, i.e. loop has 3 instruction I1, I2, I3.
Then here we are setting no-alias property like below:
I1 No-Alias-1
I2 No-Alias-1, No-Alias-2
I2 No-Alias-1, No-Alias-2, No-Alias-3
My understanding may be wrong here, if you have any better way of doing it then please suggest.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:871
@@ +870,3 @@
+ BasicBlock *BB = *I;
+ if (LI->getLoopFor(BB) == L) // Ignore blocks in subloops.
+ CurAST->add(*BB); // Incorporate the specified basic block
----------------
Why?
As we are only interested in the inner most loop, it's overhead creating AST for sub loops.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:889
@@ +888,3 @@
+ // Delete newly created loop from loop pass manager.
+ LPM.deleteLoopFromQueue(VerLoop);
+ Changed = true;
----------------
Why?
This need to be removed as we already introduced metadata to ensure not revisiting same loop.
================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:893
@@ +892,3 @@
+ // Delete allocated memory.
+ delete CurAST;
+ delete PointerSet;
----------------
These can be stack allocated.
Sure will make stack allocation.
http://reviews.llvm.org/D9151
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150619/20548614/attachment.html>
More information about the llvm-commits
mailing list