[PATCH] D13611: [Polly] Create virtual independent blocks
Johannes Doerfert via llvm-commits
llvm-commits at lists.llvm.org
Sun Feb 21 15:56:09 PST 2016
jdoerfert added a subscriber: sebpop.
jdoerfert added a comment.
I addressed all of the comments except one (the new algorithm to collect non-trivial operands).
@etherzhhb, @Meinersbur, @grosser, @Sebpop
Since this patch caused so much discussion I guess it would be nice to get two ppl to tell me it's fine to commit.
================
Comment at: include/polly/ScopInfo.h:1160
@@ -1144,4 +1159,3 @@
///
- /// Note that scalar accesses that are caused by any access in @p InvMAs will
- /// be eliminated too.
- void removeMemoryAccesses(MemoryAccessList &InvMAs);
+ /// @param OnlyMA Flag to indicate if all accesses caused by the access
+ /// instruction of @p MA should be removed or only @MA.
----------------
etherzhhb wrote:
> Does "Caused by the instruction of MA" mean the user of instruction of MA?
I created two functions now. One to remove all accesses caused by an instruction and one to remove exactly the accesses passed as arguments.
================
Comment at: include/polly/ScopInfo.h:1557
@@ +1556,3 @@
+
+ /// @brief Type for outside operands and their in SCoP users.
+ using OutsideOperandsSetTy =
----------------
etherzhhb wrote:
> why we need they in SCoP Users?
To find the access in the SCoP (if there is one). Only knowing the outside operand doesn't suffice to identify the MemoryAccess objects that we already created because of this outside operand.
================
Comment at: include/polly/ScopInfo.h:1571
@@ +1570,3 @@
+ /// operands properly.
+ using NonTrivialOperandsPairTy =
+ std::pair<InstructionSetTy, OutsideOperandsSetTy>;
----------------
etherzhhb wrote:
> ```
> a = b + c
> d = a + e
> ```
>
> I guess b, c and hopefully a are the transitive operands of d.
>
> They are also the leaves of the operand DAG rooted on d.
>
> When we recompute d, the recomputation stop at b, c, e, right? That is, all instructions inside the DAG is recomputed.
>
> So NoTrivialInstructionSet and OutsideOperandsSet together are the boundaries/leaves of the recompuation DAG, and NoTrivialInstructionSet are the instructions inside the Scop while OutsideOperandsSet are the instructions outside the Scop, right?
> I guess b, c and hopefully a are the transitive operands of d.
In the code above operands would mean {b,c,e} for the instruction d.
> They are also the leaves of the operand DAG rooted on d.
True, but whit PHI nodes we might not have a DAG and the PHIs are not necessarily leaves.
> When we recompute d, the recomputation stop at b, c, e, right? That is, all instructions inside the DAG is recomputed.
Yes. And since we do not recompute loop carried PHIs here we always have this nice DAG.
> So NoTrivialInstructionSet and OutsideOperandsSet together are the boundaries/leaves of the recompuation DAG, and NoTrivialInstructionSet are the instructions inside the Scop while OutsideOperandsSet are the instructions outside the Scop, right?
Yes, except the PHI case where we do not have a DAG and even though PHI are not leaves we put them in the NoTrivialInstructionSet.
================
Comment at: include/polly/ScopInfo.h:1575
@@ +1574,3 @@
+ /// @brief Map from in SCoP instructions to their non-trivial operands.
+ DenseMap<Instruction *, NonTrivialOperandsPairTy> NonTrivialOperandsMap;
+
----------------
etherzhhb wrote:
> I read:
> ```
> <NoTrivialInstructions (first of NonTrivialOperandsPairTy )> <OutsideOperandsSetTy (second of NonTrivialOperandsPairTy)>
> \ /
> Recomputable DAG
> | (root)
> Instruction (the key of NonTrivialOperandsMap)
> ```
>
> Actually can we check if an operand is outside Scop on the fly, such that only 1 operand set is need?
Good point, I adjusted the code to use only one set of non-trivial operands and then compute the outside operands + inside users on the fly. Thanks!
================
Comment at: lib/Analysis/ScopInfo.cpp:3109
@@ +3108,3 @@
+ continue;
+
+ Instruction *AccessInst = cast<Instruction>(MA->getAccessValue());
----------------
etherzhhb wrote:
> // Now AccessInst is a write scalar access (and not a PHI).
Added a comment.
================
Comment at: lib/Analysis/ScopInfo.cpp:3124-3149
@@ +3123,28 @@
+
+ while (!Worklist.empty()) {
+ Instruction *Inst = *Worklist.begin();
+ Worklist.erase(Inst);
+
+ if (!Visited.insert(Inst).second)
+ continue;
+
+ for (auto &InstOp : Inst->operands())
+ if (Instruction *InstOpInst = dyn_cast<Instruction>(InstOp)) {
+ if (R.contains(InstOpInst))
+ Worklist.insert(InstOpInst);
+ else
+ OutsideOperands.push_back(std::make_pair(InstOpInst, Inst));
+ }
+
+ if (Inst->mayHaveSideEffects() || Inst->mayReadFromMemory())
+ SideEffectOperands.insert(Inst);
+
+ if (!isa<PHINode>(Inst))
+ continue;
+
+ if (R.contains(Inst) && canSynthesize(Inst, &LI, &SE, &R))
+ continue;
+
+ SideEffectOperands.insert(Inst);
+ }
+
----------------
etherzhhb wrote:
> How about using the following approach, which is all over LLVM codebase, and we can create a function for it.
> ```
>
> // Depth-first traverse the operand DAG of AccessInst, until we reach:
> // * An instruction that is defined outside the current Scop (OutsideOperands )
> // * An instruction with side-effect (SideEffectOperands)
> std::set Visited;
> SmallVector<std::pair<Instruction*, Instruction::op_iterator> > VisitStack;
> VisitStack.push_back(std::make_pair(AccessInst, AccessInst->begin()));
>
> while (VisitStack.empty()) {
> auto CurNode = VisitStack.back().first;
> auto &CurChildIt = VisitStack.back().second;
>
> if (CurChildIt == CurNode->end()) {
> VisitStack.pop_back();
> continue;
> }
>
> Instruction *ChildInst = dyn_cast<Instruction>(*CurChildIt);
> ++CurChildIt;
>
> // Stop at invariances
> if (ChildInst == nullptr)
> continue;
>
> if (!Visited.insert(Inst).second)
> continue;
>
> // Invariance again.
> if (!R.contains(ChildInst)) {
> OutsideOperands.push_back(std::make_pair(InstOpInst, Inst));
> continue;
> }
>
> if (Inst->mayHaveSideEffects() || Inst->mayReadFromMemory()) {
> SideEffectOperands.insert(Inst);
> continue;
> }
>
> if (!isa<PHINode>(Inst))
> continue;
>
> if (R.contains(Inst) && canSynthesize(Inst, &LI, &SE, &R))
> continue;
>
> SideEffectOperands.insert(Inst);
> }
>
> ```
I don't get the problem you try to solve here. Why do we need a different algorithm and what is it that it does better?
================
Comment at: lib/Analysis/ScopInfo.cpp:3203
@@ +3202,3 @@
+ Instruction *OutsideOperand = OutsideOperandPair.first;
+ Instruction *OutsideUser = OutsideOperandPair.second;
+ ScopStmt *UserStmt = getStmtForBasicBlock(OutsideUser->getParent());
----------------
etherzhhb wrote:
> This user is actually inside the Scop, right?
Renamed.
================
Comment at: lib/Analysis/ScopInfo.cpp:3204
@@ +3203,3 @@
+ Instruction *OutsideUser = OutsideOperandPair.second;
+ ScopStmt *UserStmt = getStmtForBasicBlock(OutsideUser->getParent());
+ if (!UserStmt)
----------------
etherzhhb wrote:
> So what we need is only the parent block of the user that is inside the Scop?
True. I changed this code to use only one set of operands and compute everything else on the fly.
================
Comment at: lib/Analysis/ScopInfo.cpp:3209
@@ +3208,3 @@
+ // If an access to the read-only scalar is already present, continue.
+ if (Stmt.lookupValueReadOf(OutsideOperand))
+ continue;
----------------
etherzhhb wrote:
> I read:
>
> ```
> if (Stmt.containValueReadOf(OutsideOperand))
> ```
Yes. Is that a problem?
================
Comment at: lib/CodeGen/BlockGenerators.cpp:183
@@ +182,3 @@
+ copyInstruction(Stmt, OldOperandInst, BBMap, LTS, nullptr, Recompute);
+ NewOperand = BBMap[OldOperand];
+ }
----------------
etherzhhb wrote:
> If copyInstruction returns the newly copied instruction, the code
> ```
> copyInstruction(Stmt, OldOperandInst, BBMap, LTS, nullptr, Recompute);
> NewOperand = BBMap[OldOperand];
> ```
>
> Become:
> ```
> NewOperand = copyInstruction(Stmt, OldOperandInst, BBMap, LTS, nullptr, Recompute);
> ```
>
> Which looks better and easier to understand.
>
> But this can be done in another patch.
We could do that later, true.
http://reviews.llvm.org/D13611
More information about the llvm-commits
mailing list