[PATCH] [Patch] Loop Interchange Pass
hfinkel at anl.gov
hfinkel at anl.gov
Tue Feb 10 14:38:10 PST 2015
Please adjust all of the variable names to start with a capital letter, see: http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
> As Hal had suggested during RFC i went through TSVC Benchmark. Unfortunetly i didnt get time to run it but i went through the test case for loop interchange. One of the test cases s231() which was not being vectorized previously now gets vectorized. Added a similar test case in this patch.
Great, thanks!
> Also is it ok to do further development on trunk once this patch is finalized?
Yes, once this functionality is finalized, we'll move further development to trunk.
REPOSITORY
rL LLVM
================
Comment at: include/llvm/Transforms/Scalar.h:135
@@ -134,1 +134,3 @@
//
+// LoopInterchange - This pass is interchanges loops to give better cache hits.
+//
----------------
How about, "This pass interchanges loops to provide a more cache-friendly memory access patterns."
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:49
@@ +48,3 @@
+
+unsigned getInnerLoopCount(Loop &L, unsigned level) {
+ int lev = 0;
----------------
Please add a comment explaining what this function computes?
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:54
@@ +53,3 @@
+ for (Loop *InnerL : L)
+ lev = getInnerLoopCount(*InnerL, level + 1);
+ return lev;
----------------
Did you mean += ?
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:61
@@ +60,3 @@
+ // Also handle loop depth of 2 more appropriatly.
+ if (getInnerLoopCount(L, 0) == 2) {
+ Loop *Inner;
----------------
LoopInfo already has a getLoopDepth() function. Can you use that?
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:75
@@ +74,3 @@
+ PHINode *phiVar = dyn_cast<PHINode>(I);
+ if (phiVar->getNumIncomingValues() != 2)
+ continue;
----------------
This is not actually what you want. If the loop is branched to by, for example, multiple entries of the switch statement, the predecessor can be listed multiple times in the predecessor list (and, thus, you'll have more than two incoming values even though you have only 2 predecessor blocks).
I suspect that what you actually want is that there is a unique latch and a unique predecessor, so you want that L->getLoopLatch() && L->getLoopPredecessor() [neither are nullptr].
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:81
@@ +80,3 @@
+ continue;
+ const SCEV *Step = AddRec->getStepRecurrence(*SE);
+ const SCEVConstant *C = dyn_cast<SCEVConstant>(Step);
----------------
Do you also need to check that AddRec->isAffine()?
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:86
@@ +85,3 @@
+ // Found the induction variable.
+ // TODO: What if we have 2 induction variable? Currently legality makes sure
+ // we have only 1 induction variable
----------------
Let's say:
// FIXME: Handle loops with more than one induction variable. Note that, currently, legality makes sure we have only one induction variable.
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:212
@@ +211,3 @@
+ // Build up a worklist of loop pairs to analyze.
+ // [TODO] Currently only supports loop with level 2.
+ // Handle for loops greater than level 2.
----------------
I'd move this FIXME comment somewhere else, it is not particularly useful here. It is more useful to tag places that assume only two levels.
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:261
@@ +260,3 @@
+ BranchInst *outerLoopHeaderBI =
+ dyn_cast<BranchInst>(outerLoopHeader->getTerminator());
+ unsigned num = outerLoopHeaderBI->getNumSuccessors();
----------------
Either you should handle the case where the dyn_cast fails, or if it can't fail (because we've already verified that this must be a BranchInst), then use cast<> instead.
This same comment applies to many places below as well. Only use dyn_cast if the cast can fail (in which case you should handle the nullptr case). Otherwise, use cast<>.
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:273
@@ +272,3 @@
+ // The idea here is that we only need to check for usage of induction
+ // variables in header/latch to find extra instructions in the loop.Any loop
+ // independent instructions would anyhow have been sinked/hoised out of the
----------------
Space before 'Any'
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:275
@@ +274,3 @@
+ // independent instructions would anyhow have been sinked/hoised out of the
+ // loop by licm.
+ PHINode *indVar = getInductionVariable(outerLoop, SE);
----------------
licm -> LICM
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:297
@@ +296,3 @@
+ ++I) {
+ if (isa<CallInst>(I))
+ return false;
----------------
What are you actually trying to check here? Instructions with side effects? Maybe you want I->mayHaveSideEffects()?
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:309
@@ +308,3 @@
+ ++I) {
+ if (isa<CallInst>(I))
+ return false;
----------------
These checks look identical to those above, please make a function (a lambda function is fine).
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:363
@@ +362,3 @@
+ // TODO: Flow dependency can be interchanged??
+ DEBUG(dbgs() << "Flow dependence not handled");
+ return false;
----------------
Can you include *Src and *Des in these debug messages so that we can see what instructions are relevant?
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:415
@@ +414,3 @@
+ // We currently handle only 1 induction variable inside the loop. We also do
+ // not handle reductions as of now.
+ for (auto I = innerLoopHeader->begin(), E = innerLoopHeader->end(); I != E;
----------------
How are you checking for reductions here? Do you need to check that the one PHI you've found is not used outside of the loop?
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:446
@@ +445,3 @@
+ BasicBlock *BB = II->getParent();
+ if (BB == innerLoopLatch)
+ userCount++;
----------------
Why are you only counting uses in the latch block? Should the increment be in some other block, then what?
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:459
@@ +458,3 @@
+ if (!OuterLoop->getLoopPreheader() || !InnerLoop->getLoopPreheader()) {
+ DEBUG(dbgs() << "loop control flow is not understood");
+ return false;
----------------
Let's say, "Inner or outer loops lack a preheader"?
Also, for the future, adding a preheader when one is not present is pretty easy (you just need to call InsertPreheaderForLoop from llvm/Transforms/Utils/LoopUtils.h), we this is a limitation that should be removed sooner rather than later (although after the initial commit is okay).
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:500
@@ +499,3 @@
+ Instruction *UseInstr = cast<Instruction>(*IB);
+ if (isa<GetElementPtrInst>(UseInstr)) {
+ GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(UseInstr);
----------------
if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(UseInstr)) {
...
}
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:502
@@ +501,3 @@
+ GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(UseInstr);
+ if (GEP->getOperand(GEP->getNumOperands() - 1) == IV)
+ goodOrder += 1;
----------------
What happens if it is not the IV directly, but some expression of the IV? I think you'd be better off using ScalarEvolution here, get the AddRec of the GEP, and see if the "outer" AddRec is provided in terms of the SCEV of the IV (or something like that).
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:550
@@ +549,3 @@
+ // TODO: This splitting logic may not work always. Fix this.
+ splitInnerLoopLatch(innerIndexVar);
+ DEBUG(dbgs() << "splitInnerLoopLatch Done\n");
----------------
Use SplitBlock from include/llvm/Transforms/Utils/BasicBlockUtils.h?
(same for other functions below)?
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:725
@@ +724,3 @@
+ !innerLoopHeaderSuccBI)
+ assert(0 && "This should not be triggered.We have already modified parts "
+ "of the loop");
----------------
Use llvm_unreachable, not assert(0 &&
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:787
@@ +786,3 @@
+ BasicBlock *InnerLoopExitIncomingBlock = nullptr;
+ // TODO: Need to update phi nodes if any in innerLoopLatch?
+ if (innerLoopLatchBI->getSuccessor(0) == innerLoopHeader)
----------------
Why?
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:817
@@ +816,3 @@
+ I != E; ++I) {
+ if (!isa<PHINode>(I))
+ continue;
----------------
PHIs are always at the beginning of the block; once you hit the first non-PHI, you can exit the loop (you should never find another).
http://reviews.llvm.org/D7499
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list