[PATCH] [LoopInterchange] Add support to interchange loops with reductions.
Karthik Bhat
kv.bhat at samsung.com
Tue Apr 21 05:16:34 PDT 2015
Hi James, Renato,
Thanks for the comments. Updated the code to address review comments in LoopInterchange. Please find my comments inline.
Thanks and Regards
Karthik Bhat
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:258
@@ -255,3 +257,3 @@
// Checks if it is legal to interchange 2 loops.
// [Theorm] A permutation of the loops in a perfect nest is legal if and only if
// the direction matrix, after the same permutation is applied to its columns,
----------------
jmolloy wrote:
> s/Theorm/Theorem
Updated.
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:590
@@ -588,1 +589,3 @@
} // end of namespace
+bool LoopInterchangeLegality::checkAllUsesAreReductions(Instruction *Ins,
+ Loop *L) {
----------------
jmolloy wrote:
> Can you rename this "areAllUsesReductions"? it sounds more like a boolean query, which is what this is. "Check" implies some action.
Yes the new naming seems much better.Updated code.
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:595
@@ +594,3 @@
+ ReductionDescriptor RD;
+ if (!UserIns)
+ return false;
----------------
jmolloy wrote:
> You can merge these two if's:
>
> if (!UserIns || !ReductionDescriptor::isReductionPHI(UserIns, L, RD))
> return false;
>
> If you wanted to be *really* cool...
>
> return !std::any(Ins->user_begin(), Ins->user_end(), [](User *U) {
> PHINode *UserIns = dyn_cast<PHINode>(*I);
> ReductionDescriptor RD;
> return UserIns && ReductionDescriptor::isReductionPHI(UserIns, L, RD);
> });
Wow that was cool..:) got to learn and use lambda function. Updated code. But i think it shoulde be-
return !std::any_of(Ins->user_begin(), Ins->user_end(), [=](User *U) {
PHINode *UserIns = dyn_cast<PHINode>(U);
ReductionDescriptor RD;
return !UserIns || !ReductionDescriptor::isReductionPHI(UserIns, L, RD);
});
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:611
@@ -592,1 +610,3 @@
+ return true;
+ } else {
if (I->mayHaveSideEffects() || I->mayReadFromMemory())
----------------
rengolin wrote:
> nitpick, please join this "else if".
Updated code.
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:628
@@ +627,3 @@
+ return true;
+ } else {
+ if (I->mayHaveSideEffects() || I->mayReadFromMemory())
----------------
rengolin wrote:
> nitpick, please join this "else if".
Updated code.
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:696
@@ +695,3 @@
+bool LoopInterchangeLegality::populateInductionAndReductions(Loop *L) {
+ if (L->getLoopLatch() == nullptr || L->getLoopPredecessor() == nullptr)
+ return false;
----------------
jmolloy wrote:
> just:
>
> if (!L->getLoopLatch() || !L->getLoopPredecessor())
> return false;
>
Updated code.
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:707
@@ +706,3 @@
+ else
+ return false;
+ }
----------------
jmolloy wrote:
> Probably a good idea to have some debugging output here saying what PHI failed to be recognized?
Added Debugging output.
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:732
@@ +731,3 @@
+ if (BranchInst *BI = dyn_cast<BranchInst>(LatchBlock->getTerminator())) {
+ unsigned Num = BI->getNumOperands();
+ for (unsigned i = 0; i < Num; ++i) {
----------------
jmolloy wrote:
> Assert that this is 2?
Oops. This should be getNumSuccessors(). Updated code and added assertion.
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:757
@@ +756,3 @@
+ // TODO: Currently we handle only loops with 1 induction variable.
+ if (Inductions.size() != 1)
+ return true;
----------------
jmolloy wrote:
> Some debugging output saying why it failed would be nice
Done.
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:760
@@ +759,3 @@
+ InnerInductionVar = Inductions.pop_back_val();
+ Reductions.clear();
+ if (!populateInductionAndReductions(OuterLoop))
----------------
jmolloy wrote:
> I don't like this, you're mutating the content of the class for no good reason. It would be better to explicitly give Inductions and Reductions (a stack local variable) to populateInductionAndReductions(), and rename it to findInductionAndReductions()
Updated code.
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:1063
@@ -983,1 +1062,3 @@
+bool LoopInterchangeTransform::hasReductionPHI(Loop *L) {
+ BasicBlock *LoopHeader = L->getHeader();
----------------
jmolloy wrote:
> You found this out in LoopInterchangeLegality, but threw away the result. Why recalculate it here?
Modified code. Reusing value calculated by LoopInterchangeLegality in LoopInterchangeTransform.
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:1081
@@ +1080,3 @@
+ if (hasReductionPHI(InnerLoop)) {
+ // TODO: Check if the induction PHI will always be the first PHI.
+ BasicBlock *New = InnerLoopHeader->splitBasicBlock(
----------------
jmolloy wrote:
> s/TODO/FIXME
Done.
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:1093
@@ +1092,3 @@
+ Value *V = PHI->getIncomingValueForBlock(InnerLoopPreHeader);
+ for (auto UI = PHI->user_begin(), UE = PHI->user_end(); UI != UE; ++UI) {
+ PHI->replaceAllUsesWith(V);
----------------
jmolloy wrote:
> Just:
>
> for (auto U : PHI->users())
>
>
I think this for loop was redundant. I wanted to replace all uses of PHI with that of incoming value from Header. PHI->replaceAllUsesWith(V); will suffice. This loop is not required deleted the same.
================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:1138
@@ +1137,3 @@
+ for (auto I = CurrBlock->begin(); isa<PHINode>(I); ++I) {
+ PHINode *PHI = dyn_cast<PHINode>(I);
+ unsigned Num = PHI->getNumIncomingValues();
----------------
jmolloy wrote:
> Needs a bailout if I is not a PHINode.
Updated code to use "cast" instead of "dyn_cast" as I will always be a PHINode if it enters the for loop.
================
Comment at: test/Transforms/LoopInterchange/reductions.ll:3
@@ +2,3 @@
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
rengolin wrote:
> This is a generic test, it must not contain a target triple or it will fail on all aches minus x86_64. We do want to test this in ARM, MIPS, PPC, etc, so we should remove the triple and make sure it works on all buildbots.
Wont this only run if triple is x86_64-unknown-linux-gnu? But i agree i need to add some general tests as well.
================
Comment at: test/Transforms/LoopInterchange/reductions.ll:124
@@ +123,3 @@
+
+; CHECK-LABEL: @reduction_02
+; CHECK: for.body6: ; preds = %for.body6.preheader, %for.body6.split
----------------
rengolin wrote:
> These check lines are bound to fail on multiple architectures and with other optimisations coming in later, they could break the sequence or reorder instructions. You need to parse what's really relevant in the right order with the right arguments stored in variables "[[foo]]" and removed of architecture types to make sure it passes on 16/32/64 bit machines.
>
> The same is true for the other tests.
The problem here was I wanted to check if loops are interchanged properly and for that i was checking the complete structure of interchanged loop for correctness. I will try to reduce the checks.
http://reviews.llvm.org/D8314
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list