[LoopInterchange] Consider forward/backward dependency in vectorize heuristic #133672
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |||||
| #include "llvm/ADT/SmallSet.h" | ||||||
| #include "llvm/ADT/SmallVector.h" | ||||||
| #include "llvm/ADT/Statistic.h" | ||||||
| #include "llvm/ADT/StringMap.h" | ||||||
| #include "llvm/ADT/StringRef.h" | ||||||
| #include "llvm/ADT/StringSet.h" | ||||||
| #include "llvm/Analysis/DependenceAnalysis.h" | ||||||
|
|
@@ -126,7 +127,8 @@ static void printDepMatrix(CharMatrix &DepMatrix) { | |||||
| } | ||||||
| #endif | ||||||
|
|
||||||
| static bool populateDependencyMatrix(CharMatrix &DepMatrix, | ||||||
| BitVector &IsNegatedVec, unsigned Level, | ||||||
| Loop *L, DependenceInfo *DI, | ||||||
| ScalarEvolution *SE, | ||||||
| OptimizationRemarkEmitter *ORE) { | ||||||
|
|
@@ -167,7 +169,9 @@ static bool populateDependencyMatrix(CharMatrix &DepMatrix, unsigned Level, | |||||
| return false; | ||||||
| } | ||||||
| ValueVector::iterator I, IE, J, JE; | ||||||
|
|
||||||
| // Manage all found direction vectors, negated and not negated, separately. | ||||||
| StringSet<> Seen[2]; | ||||||
|
|
||||||
| for (I = MemInstr.begin(), IE = MemInstr.end(); I != IE; ++I) { | ||||||
| for (J = I, JE = MemInstr.end(); J != JE; ++J) { | ||||||
|
|
@@ -182,7 +186,8 @@ static bool populateDependencyMatrix(CharMatrix &DepMatrix, unsigned Level, | |||||
| assert(D->isOrdered() && "Expected an output, flow or anti dep."); | ||||||
| // If the direction vector is negative, normalize it to | ||||||
| // make it non-negative. | ||||||
| bool Normalized = D->normalize(SE); | ||||||
| if (Normalized) | ||||||
| LLVM_DEBUG(dbgs() << "Negative dependence vector normalized.\n"); | ||||||
| LLVM_DEBUG(StringRef DepType = | ||||||
| D->isFlow() ? "flow" : D->isAnti() ? "anti" : "output"; | ||||||
|
|
@@ -214,8 +219,12 @@ static bool populateDependencyMatrix(CharMatrix &DepMatrix, unsigned Level, | |||||
| } | ||||||
|
|
||||||
| // Make sure we only add unique entries to the dependency matrix. | ||||||
| // Negated vectors (due to normalization) are treated as separate from | ||||||
| // non negated ones. | ||||||
| if (Seen[Normalized].insert(StringRef(Dep.data(), Dep.size())).second) { | ||||||
| DepMatrix.push_back(Dep); | ||||||
| IsNegatedVec.push_back(Normalized); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
@@ -399,7 +408,7 @@ class LoopInterchangeProfitability { | |||||
| /// Check if the loop interchange is profitable. | ||||||
| bool isProfitable(const Loop *InnerLoop, const Loop *OuterLoop, | ||||||
| unsigned InnerLoopId, unsigned OuterLoopId, | ||||||
| CharMatrix &DepMatrix, const BitVector &IsNegatedVec, | ||||||
| const DenseMap<const Loop *, unsigned> &CostMap, | ||||||
| std::unique_ptr<CacheCost> &CC); | ||||||
|
|
||||||
|
|
@@ -409,9 +418,10 @@ class LoopInterchangeProfitability { | |||||
| const DenseMap<const Loop *, unsigned> &CostMap, | ||||||
| std::unique_ptr<CacheCost> &CC); | ||||||
| std::optional<bool> isProfitablePerInstrOrderCost(); | ||||||
| std::optional<bool> | ||||||
| isProfitableForVectorization(unsigned InnerLoopId, unsigned OuterLoopId, | ||||||
| CharMatrix &DepMatrix, | ||||||
| const BitVector &IsNegatedVec); | ||||||
| Loop *OuterLoop; | ||||||
| Loop *InnerLoop; | ||||||
|
|
||||||
|
|
@@ -503,8 +513,9 @@ struct LoopInterchange { | |||||
| << "\n"); | ||||||
|
|
||||||
| CharMatrix DependencyMatrix; | ||||||
| BitVector IsNegatedVec; | ||||||
| Loop *OuterMostLoop = *(LoopList.begin()); | ||||||
| if (!populateDependencyMatrix(DependencyMatrix, IsNegatedVec, LoopNestDepth, | ||||||
| OuterMostLoop, DI, SE, ORE)) { | ||||||
| LLVM_DEBUG(dbgs() << "Populating dependency matrix failed\n"); | ||||||
| return false; | ||||||
|
|
@@ -543,8 +554,8 @@ struct LoopInterchange { | |||||
| for (unsigned j = SelecLoopId; j > 0; j--) { | ||||||
| bool ChangedPerIter = false; | ||||||
| for (unsigned i = SelecLoopId; i > SelecLoopId - j; i--) { | ||||||
| bool Interchanged = processLoop(LoopList, i, i - 1, DependencyMatrix, | ||||||
| IsNegatedVec, CostMap); | ||||||
| ChangedPerIter |= Interchanged; | ||||||
| Changed |= Interchanged; | ||||||
| } | ||||||
|
|
@@ -559,6 +570,7 @@ struct LoopInterchange { | |||||
| bool processLoop(SmallVectorImpl<Loop *> &LoopList, unsigned InnerLoopId, | ||||||
| unsigned OuterLoopId, | ||||||
| std::vector<std::vector<char>> &DependencyMatrix, | ||||||
| BitVector &IsNegatedVec, | ||||||
| const DenseMap<const Loop *, unsigned> &CostMap) { | ||||||
| Loop *OuterLoop = LoopList[OuterLoopId]; | ||||||
| Loop *InnerLoop = LoopList[InnerLoopId]; | ||||||
|
|
@@ -572,7 +584,7 @@ struct LoopInterchange { | |||||
| LLVM_DEBUG(dbgs() << "Loops are legal to interchange\n"); | ||||||
| LoopInterchangeProfitability LIP(OuterLoop, InnerLoop, SE, ORE); | ||||||
| if (!LIP.isProfitable(InnerLoop, OuterLoop, InnerLoopId, OuterLoopId, | ||||||
| DependencyMatrix, IsNegatedVec, CostMap, CC)) { | ||||||
| LLVM_DEBUG(dbgs() << "Interchanging loops not profitable.\n"); | ||||||
| return false; | ||||||
| } | ||||||
|
|
@@ -1197,27 +1209,57 @@ LoopInterchangeProfitability::isProfitablePerInstrOrderCost() { | |||||
| return std::nullopt; | ||||||
| } | ||||||
|
|
||||||
| static char flipDirection(char Dir) { | ||||||
| switch (Dir) { | ||||||
| case '<': | ||||||
| return '>'; | ||||||
| case '>': | ||||||
| return '<'; | ||||||
| case '=': | ||||||
| case 'I': | ||||||
| case '*': | ||||||
| return Dir; | ||||||
| default: | ||||||
| llvm_unreachable("Unknown direction"); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Return true if we can vectorize the loop specified by \p LoopId. | ||||||
| static bool canVectorize(const CharMatrix &DepMatrix, | ||||||
| const BitVector &IsNegatedVec, unsigned LoopId) { | ||||||
| // The loop can be vectorized if there are no negative dependencies. Consider | ||||||
| // the dependency of `j` in the following example. | ||||||
| // | ||||||
| // Positive: ... = A[i][j] Negative: ... = A[i][j-1] | ||||||
| // A[i][j-1] = ... A[i][j] = ... | ||||||
| // | ||||||
| // In the right case, vectorizing the loop can change the loaded value from | ||||||
| // `A[i][j-1]`. At the moment we don't take into account the distance of the | ||||||
| // dependency and vector width. | ||||||
| // TODO: Considering the dependency distance and the vector width can give a | ||||||
| // more accurate result. For example, the following loop can be vectorized if | ||||||
| // the vector width is less than or equal to 4 x sizeof(A[0][0]). | ||||||
| for (unsigned I = 0; I != DepMatrix.size(); I++) { | ||||||
| char Dir = DepMatrix[I][LoopId]; | ||||||
| if (IsNegatedVec[I]) | ||||||
| Dir = flipDirection(Dir); | ||||||
| if (Dir != '=' && Dir != 'I' && Dir != '<') | ||||||
| return false; | ||||||
| } | ||||||
| return true; | ||||||
| } | ||||||
|
|
||||||
| std::optional<bool> LoopInterchangeProfitability::isProfitableForVectorization( | ||||||
| unsigned InnerLoopId, unsigned OuterLoopId, CharMatrix &DepMatrix, | ||||||
| const BitVector &IsNegatedVec) { | ||||||
| // If the outer loop cannot be vectorized, it is not profitable to move this | ||||||
| // to inner position. | ||||||
| if (!canVectorize(DepMatrix, IsNegatedVec, OuterLoopId)) | ||||||
| return false; | ||||||
|
|
||||||
| // If inner loop cannot be vectorized and outer loop can be then it is | ||||||
|
||||||
| // If inner loop cannot be vectorized and outer loop can be then it is | |
| // If the inner loop cannot be vectorized but the outer loop can be then it is |
[grammar]
There was a problem hiding this comment.
Fixed.
By the way, this would be nitpicky as well, but do you think the original comment is accurate? What I'm trying to say is that even if canVectorize were a perfectly accurate function (neither false-positive nor false-negative), I'm starting to think that interchanging the loops here is not necessarily profitable for enabling inner loop parallelism. For example, in the following code:
for (int i = 1; i < N; i++)
for (int j = 0; j < N; j++)
for (int k = 0; k < N; k++) {
// Assume f and g don't have side effects
use(A[i][j][f(k)]);
A[i + 1][j][g(k)] = ...;
}For the k-loop, canVectorize would return false if f and g are sufficiently complex. However, in principle, parallelizing the k-loop still seems legal in the original one. Therefore, a more accurate comment might be something like "... can be profitable to interchange the loops to enable inner loop parallelism"? (Apparently, I wrote the original comment too, so either past me or present me is wrong...)
There was a problem hiding this comment.
What is "sufficiently complex"? If DA returns "confused" then canVectorize has to return false. If it returns [< = *] the dependency is carried by the outermost loop, it does not matter what the inner loop does.
I actually don't know/undestand why canVectorize does not look at the parent loop dependencies. Possible because what the outer loops are changes with interchange. At least the loops that are surrounded by both, outer+inner could be considered.
The case you mention is interesting because it is a counterexample to the assumption that if canVectorize is pessimistic (never says a loop can be vectorized even though LoopVectorize will not for some reason), it will not cause loop exchanges that would not happen if it was not pessimistic. Anyway, in this case the j-loop looks more likely to be vectorized profitable because f(k)/g(k) indices would require more complex memory accesses. LoopVectorize can better handle i as a "strided access pattern".
I think the comment itself is correct: If the outer one could be vectorized (if moved to the inner position) but the current inner one cannot, swap the outer one to the vectorizable position. For "vectorizable" it just assumes the definition of canVectorize. Generally, even a loop is vectorizable in terms of dependencies, LoopVectorize may still consider it unprofitable to vectorize because of the instructions it contains, or the code may actually run slower after vectorization, so "profitable" was never in absolute term and hopefully understood as such by the reader. "can" does not add new information here unless we would mention such concrete situations.
There was a problem hiding this comment.
What is "sufficiently complex"? If DA returns "confused" then
canVectorizehas to return false. If it returns[< = *]the dependency is carried by the outermost loop, it does not matter what the inner loop does.
I tried to say the latter one. Just as you mentioned, I was assuming a case where DA returns [< = *].
I hadn't really been conscious of it, but as you pointed out, this is a case where pessimistic heuristics lead to an interchange that wouldn't have happened if they hadn't been pessimistic (and in this specific case, moving the j-loop would be profitable for vectorization because the memory access pattern is simpler) I personally think that the interchange should not happen in this case, since we currently don't take the vectorization cost into account. Checking dependencies of the surrounding loops seems basically like a good idea, but I'm not confident whether that might lead to other unintended transformations. Using the same cost model as LoopVectorize seems like an ideal solution, but it feels challenging.
For "vectorizable" it just assumes the definition of
canVectorize.
As for the comment here, this explanation made the most sense to me. Thanks for clarifying!
There was a problem hiding this comment.
I personally think that the interchange should not happen in this case, since we currently don't take the vectorization cost into account.
I agree, but there are limits on what we can do. At the end it is just a heuristric.
Checking dependencies of the surrounding loops seems basically like a good idea, but I'm not confident whether that might lead to other unintended transformations. Using the same cost model as LoopVectorize seems like an ideal solution, but it feels challenging.
This is a common problem that also LoopDistribute has: It is intended to enable vectorization on one more more distributed loops, but does not know whether they actually are vectorized. In other words, it has no cost model. Becausei if it does not do anything unless explicitly told to do so.
Using the profitability heuristic from LoopVectorize itself, even it it was easy, might also not what we want: Its computational cost is immense (building an entire new IR representation called VPlan) that we would not do speculatively on all loops without actually vectorizing.
There was a problem hiding this comment.
Unless you have an universal cost model that takes everything into account and predicts the execution time, each pass needs its own heuristic for what it is optimizing for. E.g. the vectorizer optmizes cycles but does not consider cache effects.
When you put it that way, it hardly seems feasible (well, if it were feasible, it would probably have been done already).
No typo; the patch tries to teach DependenceAnalysis to determine dependencies after loop fusion has taken place without applying loop fusion. Now also do that for interchange, distribution, vectorization, ....
After reading this comment, I noticed that the patch introduces additional analysis for loop fusion even though the client doesn't require it. I initially expected an argument to be added (such as depends(Src, Dst, /*ForFusion=*/true)), but that doesn't seem to be the case. Tough, controlling the analysis behavior via flags could complicate caching and reusing results across different passes.
By the way, I've recently been reading DependenceAnalysis.cpp, and noticed that; it's already quite complex and potentially buggy. I'm fairly certain it should be refactored before adding any new features.
UnrollAndJam is disabled by default. Its heuristic also does not take vectorization into account, but tires to maximize L1i cache usage.
Optimal outcome would be if the vectorizer supported outer-loop vectorization.
I don't know much about the details of the UnrollAndJam pass, but it appears to work (unintentionally?) as if outer-loop vectorization is applied in some cases, especially when combined with the SLPVectorizer (of course, I needed to specify the unroll count explicitly by pragma). So, I just thought that it might make more sense to enhance UnrollAndJam instead of interchange, for cases where outer-loop is vectorizable but inner-loop is not. And, as you said, it would be the best solution to support outer-loop vectorization in the vectorizer.
There was a problem hiding this comment.
After reading this comment, I noticed that the patch introduces additional analysis for loop fusion even though the client doesn't require it. I initially expected an argument to be added (such as
depends(Src, Dst, /*ForFusion=*/true)), but that doesn't seem to be the case. Tough, controlling the analysis behavior via flags could complicate caching and reusing results across different passes.
Whether it is for fusion is not yet decided when calling depends, but FullDependence stores the analysis for both.
By the way, I've recently been reading DependenceAnalysis.cpp, and noticed that; it's already quite complex and potentially buggy. I'm fairly certain it should be refactored before adding any new features.
The principle is straightforward; when processing one of the two fused loops, process them as the same. Since an expression can only be in one of the loops, no ambiguity arises. Only when processing the relationship between two statements you need to decide whether you want to treat them as the same or sequential loops.
I am not sure refactoring helps. Big part of why it is difficult to understand is the math. The Pair also makes it look complex, but it is just matching the access subscript dimensions after delinearization. But I also am also not very happy about adding special cases to an already complex analysis. If you do loop fusion, you may want to support more cases than loops that have excactly the same trip count.
There was a problem hiding this comment.
Whether it is for fusion is not yet decided when calling depends, but
FullDependencestores the analysis for both.
IIUC, FullDependence objects are not cached anyware. DependenceInfo is nearly stateless. Furthermore, DependenceInfo::depends returns a unique_ptr, hence we cannot cache the result as it is. That is, I think we know whether the caller is fusion or not when calling DependenceInfo::depends.
I am not sure refactoring helps. Big part of why it is difficult to understand is the math. The
Pairalso makes it look complex, but it is just matching the access subscript dimensions after delinearization. But I also am also not very happy about adding special cases to an already complex analysis. If you do loop fusion, you may want to support more cases than loops that have excactly the same trip count.
I agree that we can't do much about the mathematical complexity, but I believe the code could be made simpler. It looks to me like there's a fair amount of code duplication, especially when the same processes are executed for SrcXXX and DstXXX (e.g., here). I'm not sure whether this duplication makes the code harder to understand, but I do think it hurts maintainability. I don't believe "Don't Repeat Yourself" is always the right principle, but in this case, I think there are parts of the logic where it does apply.
However, I think the most significant problem is that we don't take wrapping into account. The approach in #116632 seems incorrect to me. We probably need to be more pessimistic with respect to wrapping. I think it makes sense to insert checks for wrap flags where necessary, which would complicate the code. I'm not sure if #146383 applies in that case, but generally speaking, adding a new feature could increase the number of factors we need to consider.
In fact, there's a case where DependenceAnalysis misses a dependency, probably due to ignoring wraps, as shown below (godbolt: https://godbolt.org/z/hsxWve8s6).
; for (i = 0; i < 4; i++)
; a[i & 1][i & 1] = 0;
define void @f(ptr %a) {
entry:
br label %loop
loop:
%i = phi i64 [ 0, %entry ], [ %i.next, %loop ]
%and = and i64 %i, 1
%idx = getelementptr [4 x [4 x i8]], ptr %a, i64 0, i64 %and, i64 %and
store i8 0, ptr %idx
%i.next = add i64 %i, 1
%exitcond.not = icmp slt i64 %i.next, 8
br i1 %exitcond.not, label %loop, label %exit
exit:
ret void
}Printing analysis 'Dependence Analysis' for function 'f':
Src: store i8 0, ptr %idx, align 1 --> Dst: store i8 0, ptr %idx, align 1
da analyze - none!
There was a problem hiding this comment.
Can you create an issue # for that case? (Or I can do so) It doesn't look nsw/nuw related though, the subscipts are well within i64 range.
I remember having had issues with #116632 but apparently I have been convinced otherwise.
Would be looking forward to cleanup PRs on DA.
There was a problem hiding this comment.
Ah, sorry, the issue already exists: #148435 (comment)
I think this is a kind of wrapping problem. IIRC, the %and is represented as {false,+,true}<%loop>, which would wrap. But DA casts it to i64 and ultimately overlooks the wrapping.
(While I'm at it, I'll share the other issues I found: #149977, #149501, #149991).
Would be looking forward to cleanup PRs on DA.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[serious] Both of these have positive dependence distance, jsut the first is a WAR (anti-)dependence, the second is a RAW (flow)-dependence.
The
i-loop variable is irrelevant here since always the same, so the dependence distance can only be non-negative.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm confusing the terms "positive/negative" and "forward/backward". IIUIC, I meant to say here that a forward RAW dependence doesn't prevent vectorization.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
positive/negative refers to the difference of (normalized) loop induction values (
i,j)forward/backward refers to the execution order of statements within the loop body. A forward dependence is just the normal execution flow, e.g.
A backward dependence is when the source of a dependences is a statement that is located after the destination in the loop body, necessarily from a previous iteration:
In the polyhedral model one just assigns numbers to the sequence of statements in the loop which allows doing calculations over statement order as if it was another loop:
In this view, a forward dependency is when the innermost distance vector element (i.e.
k) is positive, and a backward dependency is when the innermost dependence vector element is negative. I find this view helpful.As mentioned, the execution order of statements is ambigious if the body is not straight-line code. I had lenghty discussions on the OpenMP language committee about it. For instance, assume an if/else construct:
Is this a forward or backward dependency? It is kind-of ill-defind because within the same body iteration, only one of the statements is ever executed, so there is no order between them. This becomes clearer if you consider that the following has the very same semantics:
So it does not matter? Well it does if you vectorize using predicated instructions. You can vectorize the latter, with simd width of 2 (and more if you allow store-to-load forwarding), but you cannot vectorize the former, at least not by just replacing every statement with its vector equivalent. If you want to keep it simple, only consider straight-line code within a single BB.
The entire forward/backward nomenclature also breaks down if you allow non-perfectly nested loops.
I also dislike calling those "lexically" forward/backward. I can use e.g. gotos to change the lexical order of statements:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the really thorough explanation! It took me some time to fully understand it, but I believe I got what you meant.
Yes, that made it much clearer. Thanks! And thanks to this, I finally see why you recommended to represent the information about whether a dependency is forward or not by the last element of the direction vector.
(This is probably off-topic, but seeing this made me realize that I was almost confusing the "lexicographical order" in the context of direction vectors with the "lexical forward/backward" in LAA.)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are related:
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm biased, but when I here "dictionary order", I kind of imagine the order on a Cartesian product...