-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[DA] Check monotonicity for subscripts #154527
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@Meinersbur I'd like to get your thoughts on whether this approach seems reasonable. What do you think about this? Also, I believe similar checks are needed in other parts of the DA as well, for example this function. |
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.
Some thoughts:
- This seems to only support AddRecExpr directly, I was thinking about something recursive that may contain an AddRecExpr, such as an SignExtend/ZeroExtend/Division/... of an SCEVAddRecExpr. The most common operations (mul/add with invariant) may indeed usually be folded into the SCEVAddRecExpr (always?). SCEVTruncate maybe illustrates that the "monotonic" property not only applies to SCEVAddRecExpr.
trunc 0x101 to i8 -> 0x01
trunc 0x203 to i8 -> 0x03
trunc 0x302 to i8 -> 0x02
Say the values 0x101, 0x203, 0x302 are the iteration values a loop1. The maximum of the trunc expression is 0x03
, which is neither the value of the initial AddRecExpr loop iteration, nor its last. That is, the range of indices in an array subscipt expression A[(char)i]
is not 0x01 to 0x02.
- I would have thougth of getting the min/max of an expression as a separate operation. Ideally, the simplication of SMin/SMax expression could be done by ScalarEvolution itself through canonicalization/folding.
Footnotes
-
Not sure how it could be encoded as an SCEVAddRecExpr or combination of SCEVs, but the point is that it monotonically increasing ↩
/// is monotonic. The term "monotonic" means that all AddRec Exprs in \p Expr | ||
/// doesn't wrap in signed sense. When it is monotonic, the minimum and | ||
/// maximum values of \p Expr are stored in \p Min and \p Max, respectively. | ||
bool isMonotonicSCEV(const SCEV *Expr, const SCEV *&Min, const SCEV *&Max, |
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.
Document parameters?
Consider adding "signed" to the name. Currently DA assumes everything is a signed SCEV, but maybe we change that one day.
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.
Separate the implementation, and added "signed" to the name.
const SCEV *&Max, ScalarEvolution *SE, | ||
const Loop *OutermostLoop, IntegerType *Ty, | ||
const Value *Ptr) const { | ||
const SCEVAddRecExpr *AddRec = dyn_cast<SCEVAddRecExpr>(Expr); |
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.
Consider using the SCEVVisitor
pattern. It was made for such uses.
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, it completely slipped my mind.
✅ With the latest revision this PR passed the C/C++ code formatter. |
ac9bbc6
to
5063efc
Compare
Thanks for the feedback!
Changed the implementation to use the
It actually seems monotonic, but I'm not sure the correct way to handle such cases... Let me think about it for a moment.
Splitted it in another class So, I believe the overall approach is not too far off, so for now I'll check the tests and clean up the code. I’d like to ask one question: I feel like these processes (monotonic checks, validation for delinearization, etc.) should be tested separately from other parts of DA. What do you think? Would that be a bit excessive? |
return MonotonicityType::MaySignedWrap; | ||
Result = MonotonicityType::Constant; | ||
break; | ||
} |
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 dont understand one thing here. If the entire SCEV is NSW, why do we need to check if its NSW for individual operands? Do you have specific case in mind?
Also, I am trying to understand what MonotonicityType::Monotonic
really means. Just going by the mathematical definition of monotonicity,
- why
Monotonic + Monotonic
should be MaySignedWrap? Shouldnt it be Monotonic? - why
Monotonic + Constant
should be Constant? Shouldnt it be Monotonic?
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.
First of all, probably the name is misleading. MaySignedWrap
should be renamed to something like Unknown
.
If the entire SCEV is NSW, why do we need to check if its NSW for individual operands? Do you have specific case in mind?
I was imagining an example like {0,+,%m}<%loop> + {0,+,%n}<%loop>
, but I'm not sure if such a representation can actually exist. If ScalarEvolution guarantees that this form is always folded into something like {0,+,(%m+%n)}<%loop>
, then maybe this is unnecessary. But if not, I'm not confident it's safe when each operand can potentially overflow (although DA doesn't support this kind of format)
Just going by the mathematical definition of monotonicity
DA breaks exactly due to the gap between mathematical theory and LLVM IR semantics.
- why
Monotonic + Monotonic
should be MaySignedWrap? Shouldnt it be Monotonic?
To clearly distinguish between Constant
and Monotonic
. I was considering a case like {0,+,1}<%loop> + {0,+,-1}<%loop>
. This always seems to evaluate to 0 (i.e., a Constant
), but again, I'm not sure if such a representation can exist in SCEV.
- why
Monotonic + Constant
should be Constant? Shouldnt it be Monotonic?
I think this is just a simple implementation mistake. Thanks for pointing it out.
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.
First of all, probably the name is misleading. MaySignedWrap should be renamed to something like Unknown
ok, please change to Unknown or CouldNotCompute. MaySignedWrap
is confusing
example like {0,+,%m}<%loop> + {0,+,%n}<%loop>
If the entire expression is nuw/nsw then individual SCEVs must follow the same pattern but vice-versa cant be true(this may wrap).
that this form is always folded into something like {0,+,(%m+%n)}<%loop>
this is not true because when its split form, each AddRed can have different values . But with (%m+%n) , every itr is multiple of (%m+%n)
{0,+,1}<%loop> + {0,+,-1}<%loop>
this expr can have values 0(=0+0), -1(=0-1), 1(=1+0), 0(=1-1). This is definitely not a constant. So, this should be Unknown
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.
What I'm not entirely sure about is whether, given the following IR, the SCEV corresponding to %mn_i
is guaranteed to be {0,+,(%m + %n)}<%loop>
rather than {0,+,%m}<%loop> + {0,+,%n}<%loop>
loop:
%i = phi i64 [ 0, %entry ], [ %i.inc, %loop ]
%m_i = mul nsw i64 %m, %i
%n_i = mul nsw i64 %n, %i
%mn_i = add nsw i64 %m_i, %n_i
...
If not, then I don't think we can say "Monotonic + Monotonic = Monotonic", since %m
could be equal to -1 * %n
, in which case %mn_i
would always be zero. I don't know whether a constant value is considered to "monotonic" in general mathematical theory, but it is a corner case in the context of 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.
if m
is invariant in current loop then it should be {0,+,(%m + %n)}<%loop>
and vice-versa if n
comes from outer loop
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.
Okay, then I think the logic can be simplified.
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.
@kasuga-fj Expressions like {0,+,%m}<%loop> + {0,+,%n}<%loop>
are possible if SCEV either hits the arithmetic depth limit, or the huge expression limit.
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.
@nikic I see, thanks for letting me know.
if (StepRes != MonotonicityType::Constant || !SE->isKnownNonZero(Step)) | ||
return MonotonicityType::MaySignedWrap; | ||
|
||
bool IsNSW = [&] { |
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.
should this be IsNoWrap
?
return MonotonicityType::MaySignedWrap; | ||
|
||
bool IsNSW = [&] { | ||
if (Expr->hasNoSignedWrap()) |
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.
shouldnt you check for NoWrap? The expression, if unsigned, may not fit into the signed range
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.
This is intentional. DA currently mixes signed and unsigned interpretations, which can lead to incorrect results. One of the main goals of this PR (and future ones) is to unify all integer interpretations in DA under a signed one.
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.
if it only has nuw
, the analysis would go wrong.
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.
Yes, and the expected behavior is to detect such expressions and bail out of the analysis. The subsequent checks attempt to prove properties similar to nsw when it's not explicitly attached, although I'm not sure they're truly necessary (I've not tested enough yet).
SCEVSignedMonotonicityChecker::visitUnknown(const SCEVUnknown *Expr) { | ||
return SE->isLoopInvariant(Expr, OutermostLoop) | ||
? MonotonicityType::Constant | ||
: MonotonicityType::MaySignedWrap; |
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.
this is SCEVUnknown in the context of current loop. Why do we need to evaluate in the scope of outermost loop?
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.
Just to align with the current behavior.
llvm-project/llvm/lib/Analysis/DependenceAnalysis.cpp
Lines 854 to 867 in db02476
// Returns true if Expression is loop invariant in LoopNest. | |
bool DependenceInfo::isLoopInvariant(const SCEV *Expression, | |
const Loop *LoopNest) const { | |
// Unlike ScalarEvolution::isLoopInvariant() we consider an access outside of | |
// any loop as invariant, because we only consier expression evaluation at a | |
// specific position (where the array access takes place), and not across the | |
// entire function. | |
if (!LoopNest) | |
return true; | |
// If the expression is invariant in the outermost loop of the loop nest, it | |
// is invariant anywhere in the loop nest. | |
return SE->isLoopInvariant(Expression, LoopNest->getOutermostLoop()); | |
} |
But I'm not sure whether checking invariance in the current loop is sufficient or not.
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.
think conversely. If this was MonotonicityType::Constant
, would this function be ever called? It should still be MonotonicityType::MaySignedWrap
|
||
MonotonicityType SrcMonotonicity = | ||
SCEVSignedMonotonicityChecker(SE, OutermostLoop, SrcPtr) | ||
.visit(SrcSubscripts[I]); |
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.
Same comment. Why evaluate only in the scope of Outermost loop? Shouldnt this be in context of current loop?
I was wondering that isnt checking only geps for wrapping (i.e. nusw) sufficient ? Also, is MinMax calculator really needed? For cases like #149501, we just need delta and bounds, right? Are there any other cases where min/max are needed seperately? |
It's insufficient. Please refer to relevant test cases, such as https://github.com/llvm/llvm-project/blob/296163f85dfc6a7f85972f5385ff85e67738a956/llvm/test/Analysis/DependenceAnalysis/DADelin.ll. For example, in
Yes, it is necessary to validate the delinearization result. Please note that this PR aims to address the broader issue that "DA doesn't account for wrapping accesses", not just the specific case I gave in #149501. |
Constant, ///< The expression is constant. If a SCEV is classified as | ||
///< Constant, it also implies that it doesn't contain any | ||
///< arithmetic operations that may cause signed wrap. |
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.
What is the utility of Constant
? isa<SCEVConstant>(...)
should be sufficient. Did you mean "invariant" to a specific loop?
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.
Yes, "invariant" seems more reasonable here.
Monotonic, ///< The expression is monotonically increasing or decreasing. This | ||
///< is exclusive of Constant. That is, we say an SCEV is Monotonic | ||
///< iff it contains at least one AddRec where its step reccurence | ||
///< value is non-zero. |
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 the description should mention it is relative to a specific loop. If it does not contain an SCEVAddRec, it could still contain a SCEVUnkown that is non-invariant in that loop.
It becomes interesting if you have a SCEVAddRec of a nested loop in there. How do you think those should be handled? E.g. the specific loop is counting up but the nested loop is counting down.
for (int i = n; i >= 0; --i) {
int j = 0;
for (; j < m; ++j)
A[i + j] = ...;
B[i + j] = ...;
}
Is the SCEV for A[i + j]
monotonic? I think it for B[i + j]
because at that point j==m
is invariant, so the min is 0 + m
and the max is n + m
.
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'm not entirely sure whether the term "monotonic" is appropriate here, but I believe A[i + j]
in the example is "valid" in the context of DA. I think what I'm trying to conceptualize is something like how multilinear relates to linear -- perhaps something like 'multimonotonic' for monotonic?
So, I intended these properties to be considered with respect to the outermost loop, rather than any specific loop within the loop nest. In any case, I'm now feeling strongly that I should add some tests to illustrate the expected behavior.
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.
Added some cases to monotonic.ll
to illustrate my thoughts. I used debug outputs for now, but I'd prefer to avoid if it possible...
|
||
private: | ||
ScalarEvolution *SE; | ||
const Loop *OutermostLoop; |
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.
Possibly remove the "Outermost", does not necessarily need to be an outermost loop.
Two points:
Instead of a separate monotonicity checker, enhance SCEV's existing getRange() methods to detect wrapping more accurately and integrate this into the existing DA wrapping checks. |
Lets suppose that |
I think these checks include some logic specific to DA, such as inferring properties from
No. I don't think that's true in DA. Delienarization can decompose the original offset into multiple subscripts that are not "equivalent" to how the offset actually computed, which makes the problem complicated. Consider the following case, which I raised in #152566: ; void f(char *a, unsigned long long d) {
; if (d == UINT64_MAX)
; for (unsigned long long i = 0; i != d; i++)
; a[i * (d + 1)] = 42;
; }
define void @f(ptr %a, i64 %d) {
entry:
%guard = icmp eq i64 %d, -1
br i1 %guard, label %loop.preheader, label %exit
loop.preheader:
%stride = add nsw i64 %d, 1 ; since %d is -1, %stride is 0
br label %loop
loop:
%i = phi i64 [ 0, %loop.preheader ], [ %i.next, %loop ]
%offset = phi i64 [ 0, %loop.preheader ], [ %offset.next, %loop ]
%idx = getelementptr inbounds i8, ptr %a, i64 %offset
store i8 42, ptr %idx
%i.next = add nuw i64 %i, 1
%offset.next = add nsw nuw i64 %offset, %stride
%cond = icmp eq i64 %i.next, %d
br i1 %cond, label %exit, label %loop
exit:
ret void
} The
Yeah, the |
Firstly, this entire program is UB since d+1 wraps around. So, I am not much concerned about this specific example. Secondly, the array dimensions should be A[d+1][d]. It has deduced the delinearized dimensions as
So, I think this is not the right example that says if you have nuw/nsw flags on GEP, you cant guarantee nuw/nsw flags on individual subscripts |
I don't think that's correct. In the C language, wrapping behavior for unsigned integers is well-defined. In LLVM IR, unsigned wrapping is also permitted unless the EDIT: Even if the
The deduced dimensions are
The |
Ah, my bad about wrapping behavior for unsigned .
Does it always need to imply nsw ? I am slightly skeptical. Consider this. Its already accessing UINT64_MAX which is beyond signed representation. Or am I misinterpreting something here? Anyway, coming back to the example, array dimensions can be expressed as A[d+1][d].
|
I'll separate the UPDATE: Removed |
Sorry, I didn't catch what you meant. As far as I can tell, LLVM IR uses two's complement representation for integers.
The IR I presented was handwritten. I don't know whether the actual compiler infers those flags.
Please refer to the delinearization function. Just to clarify, this is a heuristic based on the input IR and does not reflect how the original program accesses the array. |
SCEV's range analysis only returns But I found related functionality in SE which is |
It seems to be a private function, and at a glance, we have to set |
I found a case where ; for (int i = 0; i < 100; i++)
; a[i & 1] = 0;
define void @f(ptr %a) {
entry:
br label %loop
loop:
%i = phi i64 [ 0, %entry ], [ %i.inc, %loop ]
%i.1 = phi i1 [ false, %entry ], [ %i.1.inc, %loop ]
%offset = sext i1 %i.1 to i64
%idx = getelementptr i8, ptr %a, i64 %offset
store i8 0, ptr %idx
%i.inc = add i64 %i, 1
%i.1.inc = add i1 %i.1, true
%exitcond = icmp eq i64 %i.inc, 100
br i1 %exitcond, label %exit, label %loop
exit:
ret void
}
As I tested locally, |
The memory access wrap hasn't been properly handled in DA. As a first step, this patch introduces the concept of "monotonicity" and applies it to validate the result of delinearization.
Related: #151326 (comment)