[PATCH] D13338: [Polly] Consolidate invariant loads
Michael Kruse via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 1 11:13:53 PDT 2015
Meinersbur added a comment.
In http://reviews.llvm.org/D13338#257549, @jdoerfert wrote:
> In http://reviews.llvm.org/D13338#257504, @Meinersbur wrote:
>
> > Could you please describe what the code is doing not only in the commit message, but also as source code comment?
>
>
> I think the source is well documented. If you disagree please inline a comment so I know what part you refer too.
The commit message describes 3 phases, but there are just 1.5 notable new comments. What about the other phases?
I added some inline comments where I think you could write a bit more. (These are not questions you need to answer to me, I got them from either some other comment or the commit log).
Mmh, I mixed them with my other remarks that I found.
> > Why is ScopDetection involved at all?
>
>
> Because in ScopInfo we cannot build the equivalence classes until the SCoP is completed and to build the SCoP in resonable time we need equivalence classes.
> For example in the SCEVAffinator (that is used throughout the SCoP creation) we need to normalize required invariant load parameters otherwise we would introduce different parameters for each invariant load.
> To normalize these parameters we already need equivalence classes but in the expression that is translated at that point there might only be a reference to one of the invariant loads (most certainly not to all that are equivalent).
> Thus, to determine the representing element for an equivalence class we need to know all elements of it before we use the SCEVAffinator for the first time.
> The only way to build the equivalence classses before we use the SCEVAffinator is to do it in the ScopDetection where we actually see all required invariant loads.
Correct me if I am wrong, SCEVAffinator is only used by the ScopInfo pass. ScopInfo also get the list of invariant using getRequiredInvariantLoads(). It can by itself create the equivalence classes by going through the map.
This might be additional work because ScopDetection in the patch does the equivalence classes already on the fly. However, it would be better for layering as ScopDetection shouldn't care about hoisting; it just determines the size of scop regions.
> > Shouldn't it be ScopInfo alone which decides what that Scop's parameters are?
>
>
> ScopInfo actually never "really" decides what the parameters are, it only normalizes them to a certain degree (and now even more). The parameters are collected and given to the SCoP by the SCEVAffinator and the SCEVValidator.
Isn't it ScopInfo::addParams() which collect the parameters?
================
Comment at: include/polly/ScopDetection.h:150
@@ -149,3 +149,3 @@
/// @brief Map to remember loads that are required to be invariant.
- DenseMap<const Region *, InvariantLoadsSetTy> RequiredInvariantLoadsMap;
+ DenseMap<const Region *, InvariantLoadsClassesTy> RequiredInvariantLoadsMap;
----------------
What are they remembered for?
================
Comment at: include/polly/ScopDetection.h:188
@@ -187,3 +187,3 @@
/// @brief Loads that need to be invariant during execution.
- InvariantLoadsSetTy &RequiredILS;
+ InvariantLoadsClassesTy &RequiredILC;
----------------
What are the equivalence classes? Why are there equivalence classes?
================
Comment at: include/polly/ScopDetection.h:254
@@ -253,3 +253,3 @@
- /// @brief Check if the given loads could be invariant and can be hoisted.
+ /// @brief Check if the required invariant loads can be hoisted.
///
----------------
What is the condition for that?
================
Comment at: include/polly/ScopDetection.h:402
@@ -401,2 +401,3 @@
/// @brief Return the set of required invariant loads for @p R.
+ const InvariantLoadsClassesTy *
----------------
Why are they required?
================
Comment at: include/polly/ScopInfo.h:936
@@ -932,3 +935,3 @@
void hoistMemoryAccesses(MemoryAccessList &InvMAs,
- InvariantAccessesTy &TargetList);
+ InvariantAccessesTy &InvariantAccesses);
----------------
Why the rename?
================
Comment at: include/polly/ScopInfo.h:1219
@@ -1215,2 +1218,3 @@
/// - removal of empty statements (due to invariant load hoisting)
+ /// - consolidation of invariant loads of the same address.
void simplifySCoP();
----------------
If those are two actions, why not put them into different functions?
I can't find the changes for simplifySCoP().
================
Comment at: include/polly/ScopInfo.h:1241
@@ +1240,3 @@
+ /// @brief Return the required invariant load equivalence classes.
+ const InvariantLoadsClassesTy &getRIL() const;
+
----------------
I prefer the longer name from ScopDetection
================
Comment at: include/polly/Support/SCEVAffinator.h:57
@@ -56,1 +56,3 @@
+ /// @param BB The block in which @p E is executed.
+ /// @param ILC The invariant loads equivalence classes of the SCoP.
///
----------------
What is it used for?
================
Comment at: include/polly/Support/SCEVValidator.h:51
@@ -50,3 +50,3 @@
llvm::ScalarEvolution &SE, const llvm::Value *BaseAddress = 0,
- InvariantLoadsSetTy *ILS = nullptr);
+ InvariantLoadsClassesTy *ILC = nullptr);
std::vector<const llvm::SCEV *>
----------------
@param missing
================
Comment at: include/polly/Support/ScopHelper.h:46
@@ -45,3 +45,3 @@
-/// @brief Type for a set of invariant loads.
+/// @brief Type for a __ordered__ set of invariant loads.
using InvariantLoadsSetTy = llvm::SetVector<llvm::LoadInst *>;
----------------
What is the order?
================
Comment at: include/polly/Support/ScopHelper.h:49
@@ -48,1 +48,3 @@
+/// @brief Type for equivalence classes of invariant loads.
+using InvariantLoadsClassesTy =
----------------
What is the key?
================
Comment at: include/polly/Support/ScopHelper.h:152
@@ +151,3 @@
+
+/// @brief Get the representing SCEV for invariant loads if applicable.
+///
----------------
When is it applicable? What is the special property of the representive SCEV?
================
Comment at: lib/Analysis/ScopDetection.cpp:302
@@ -301,2 +301,3 @@
bool ScopDetection::onlyValidRequiredInvariantLoads(
+ InvariantLoadsClassesTy &RequiredILC, DetectionContext &Context) const {
----------------
Describe under which condition the invariant valid load is required
================
Comment at: lib/Analysis/ScopDetection.cpp:326
@@ -321,3 +325,3 @@
- if (!onlyValidRequiredInvariantLoads(AccessILS, Context))
+ if (!onlyValidRequiredInvariantLoads(AccessILC, Context))
return false;
----------------
return onlyValidRequiredInvariantLoads(AccessILC, Context)
================
Comment at: lib/Analysis/ScopDetection.cpp:705
@@ -701,1 +704,3 @@
+ const SCEV *PointerSCEV = SE->getSCEV(LInst->getPointerOperand());
+ Context.RequiredILC[PointerSCEV].insert(LInst);
continue;
----------------
This is 1)?
================
Comment at: lib/Analysis/ScopDetection.cpp:1148
@@ -1142,3 +1147,3 @@
NonAffineSubRegionSetTy DummyNonAffineSubRegionSet;
- InvariantLoadsSetTy DummyILS;
+ InvariantLoadsClassesTy DummyILC;
DetectionContext Context(const_cast<Region &>(R), *AA,
----------------
Is it dummy (could you pass NULL instead)? Or does it serve as scratch storage?
================
Comment at: lib/Analysis/ScopInfo.cpp:1368
@@ -1366,3 +1367,3 @@
void ScopStmt::hoistMemoryAccesses(MemoryAccessList &InvMAs,
- InvariantAccessesTy &TargetList) {
+ InvariantAccessesTy &InvariantAccesses) {
----------------
Why the rename?
================
Comment at: lib/Analysis/ScopInfo.cpp:1424
@@ +1423,3 @@
+ // class at the end of InvariantAccesses.
+ // TODO: This is quadratic in the number invariant accesses.
+ LoadInst *LInst = cast<LoadInst>(MA->getAccessInstruction());
----------------
Ideas how to improve this?
================
Comment at: lib/Analysis/ScopInfo.cpp:1437
@@ +1436,3 @@
+ isl_set *IAClassDomainCtx =
+ isl_set_union(IA.second, isl_set_copy(DomainCtx));
+ IAClass.push_front(std::make_pair(MA, IAClassDomainCtx));
----------------
Is this 2)? Can you describe why it is the union of the two?
================
Comment at: lib/Analysis/ScopInfo.cpp:1476
@@ -1446,1 +1475,3 @@
__isl_give isl_id *Scop::getIdForParam(const SCEV *Parameter) const {
+ // Normalize invariant loads first to get the correct parameter SCEV.
+ Parameter = normalizeInvariantLoadSCEV(Parameter, *getSE(), getRIL());
----------------
Why is one parameter correct but not the other?
================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:908
@@ -907,3 +907,3 @@
- const auto &InvAccList = S.getInvariantAccesses();
- if (InvAccList.empty())
+ const auto &InvariantAccesses = S.getInvariantAccesses();
+ if (InvariantAccesses.empty())
----------------
Why the rename? Doesn't "auto" know by itself that it's a const reference?
================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:930
@@ -928,2 +929,3 @@
Instruction *AccInst = MA->getAccessInstruction();
Value *PreloadVal = preloadInvariantLoad(*MA, Domain, Build);
+ for (const InvariantAccessTy &IA : IAClass)
----------------
This is 3) ?
================
Comment at: lib/Support/ScopHelper.cpp:385
@@ +384,3 @@
+const SCEV *
+polly::normalizeInvariantLoadSCEV(const SCEV *S, ScalarEvolution &SE,
+ const InvariantLoadsClassesTy &ILC) {
----------------
Describe the conditions
http://reviews.llvm.org/D13338
More information about the llvm-commits
mailing list