[BasicAA] Track nuw through decomposed expressions #106512
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When we decompose the GEP offset expression, and the arithmetic is not performed using nuw operations, we cannot retain the nuw flag on the decomposed GEP. For example, if we have `gep nuw p, (a-1)`, this is not at all the same as `gep nuw (gep nuw p, a), -1`. Fix this by tracking NUW through linear expression decomposition, similarly to what we already do for the NSW flag.
llvmbot
added
the
llvm:analysis
|
@llvm/pr-subscribers-llvm-analysis Author: Nikita Popov (nikic) ChangesWhen we decompose the GEP offset expression, and the arithmetic is not performed using nuw operations, we cannot retain the nuw flag on the decomposed GEP. For example, if we have Fix this by tracking NUW through linear expression decomposition, similarly to what we already do for the NSW flag. This fixes the miscompilation reported in #105496 (comment). Full diff: https://github.com/llvm/llvm-project/pull/106512.diff 2 Files Affected:
diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index 72db28929c0c37..a00ed7530ebc4c 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -375,24 +375,28 @@ struct LinearExpression {
APInt Scale;
APInt Offset;
+ /// True if all operations in this expression are NUW.
+ bool IsNUW;
/// True if all operations in this expression are NSW.
bool IsNSW;
LinearExpression(const CastedValue &Val, const APInt &Scale,
- const APInt &Offset, bool IsNSW)
- : Val(Val), Scale(Scale), Offset(Offset), IsNSW(IsNSW) {}
+ const APInt &Offset, bool IsNUW, bool IsNSW)
+ : Val(Val), Scale(Scale), Offset(Offset), IsNUW(IsNUW), IsNSW(IsNSW) {}
- LinearExpression(const CastedValue &Val) : Val(Val), IsNSW(true) {
+ LinearExpression(const CastedValue &Val)
+ : Val(Val), IsNUW(true), IsNSW(true) {
unsigned BitWidth = Val.getBitWidth();
Scale = APInt(BitWidth, 1);
Offset = APInt(BitWidth, 0);
}
- LinearExpression mul(const APInt &Other, bool MulIsNSW) const {
+ LinearExpression mul(const APInt &Other, bool MulIsNUW, bool MulIsNSW) const {
// The check for zero offset is necessary, because generally
// (X +nsw Y) *nsw Z does not imply (X *nsw Z) +nsw (Y *nsw Z).
bool NSW = IsNSW && (Other.isOne() || (MulIsNSW && Offset.isZero()));
- return LinearExpression(Val, Scale * Other, Offset * Other, NSW);
+ bool NUW = IsNUW && (Other.isOne() || MulIsNUW);
+ return LinearExpression(Val, Scale * Other, Offset * Other, NUW, NSW);
}
};
}
@@ -408,7 +412,7 @@ static LinearExpression GetLinearExpression(
if (const ConstantInt *Const = dyn_cast<ConstantInt>(Val.V))
return LinearExpression(Val, APInt(Val.getBitWidth(), 0),
- Val.evaluateWith(Const->getValue()), true);
+ Val.evaluateWith(Const->getValue()), true, true);
if (const BinaryOperator *BOp = dyn_cast<BinaryOperator>(Val.V)) {
if (ConstantInt *RHSC = dyn_cast<ConstantInt>(BOp->getOperand(1))) {
@@ -444,6 +448,7 @@ static LinearExpression GetLinearExpression(
E = GetLinearExpression(Val.withValue(BOp->getOperand(0), false), DL,
Depth + 1, AC, DT);
E.Offset += RHS;
+ E.IsNUW &= NUW;
E.IsNSW &= NSW;
break;
}
@@ -451,13 +456,14 @@ static LinearExpression GetLinearExpression(
E = GetLinearExpression(Val.withValue(BOp->getOperand(0), false), DL,
Depth + 1, AC, DT);
E.Offset -= RHS;
+ E.IsNUW = false; // sub nuw x, y is not add nuw x, -y.
E.IsNSW &= NSW;
break;
}
case Instruction::Mul:
E = GetLinearExpression(Val.withValue(BOp->getOperand(0), false), DL,
Depth + 1, AC, DT)
- .mul(RHS, NSW);
+ .mul(RHS, NUW, NSW);
break;
case Instruction::Shl:
// We're trying to linearize an expression of the kind:
@@ -472,6 +478,7 @@ static LinearExpression GetLinearExpression(
Depth + 1, AC, DT);
E.Offset <<= RHS.getLimitedValue();
E.Scale <<= RHS.getLimitedValue();
+ E.IsNUW &= NUW;
E.IsNSW &= NSW;
break;
}
@@ -697,7 +704,8 @@ BasicAAResult::DecomposeGEPExpression(const Value *V, const DataLayout &DL,
// If the integer type is smaller than the index size, it is implicitly
// sign extended or truncated to index size.
bool NUSW = GEPOp->hasNoUnsignedSignedWrap();
- bool NonNeg = NUSW && GEPOp->hasNoUnsignedWrap();
+ bool NUW = GEPOp->hasNoUnsignedWrap();
+ bool NonNeg = NUSW && NUW;
unsigned Width = Index->getType()->getIntegerBitWidth();
unsigned SExtBits = IndexSize > Width ? IndexSize - Width : 0;
unsigned TruncBits = IndexSize < Width ? Width - IndexSize : 0;
@@ -706,9 +714,11 @@ BasicAAResult::DecomposeGEPExpression(const Value *V, const DataLayout &DL,
// Scale by the type size.
unsigned TypeSize = AllocTypeSize.getFixedValue();
- LE = LE.mul(APInt(IndexSize, TypeSize), NUSW);
+ LE = LE.mul(APInt(IndexSize, TypeSize), NUW, NUSW);
Decomposed.Offset += LE.Offset.sext(MaxIndexSize);
APInt Scale = LE.Scale.sext(MaxIndexSize);
+ if (!LE.IsNUW)
+ Decomposed.NWFlags = Decomposed.NWFlags.withoutNoUnsignedWrap();
// If we already had an occurrence of this index variable, merge this
// scale into it. For example, we want to handle:
@@ -719,7 +729,8 @@ BasicAAResult::DecomposeGEPExpression(const Value *V, const DataLayout &DL,
areBothVScale(Decomposed.VarIndices[i].Val.V, LE.Val.V)) &&
Decomposed.VarIndices[i].Val.hasSameCastsAs(LE.Val)) {
Scale += Decomposed.VarIndices[i].Scale;
- LE.IsNSW = false; // We cannot guarantee nsw for the merge.
+ // We cannot guarantee no-wrap for the merge.
+ LE.IsNSW = LE.IsNUW = false;
Decomposed.VarIndices.erase(Decomposed.VarIndices.begin() + i);
break;
}
diff --git a/llvm/test/Analysis/BasicAA/gep-nuw-alias.ll b/llvm/test/Analysis/BasicAA/gep-nuw-alias.ll
index b80a457f85176c..a5f1c1c747cc3f 100644
--- a/llvm/test/Analysis/BasicAA/gep-nuw-alias.ll
+++ b/llvm/test/Analysis/BasicAA/gep-nuw-alias.ll
@@ -212,3 +212,106 @@ define void @both_var_idx(ptr %p, i64 %i, i64 %j) {
ret void
}
+
+; CHECK-LABEL: add_no_nuw
+; CHECK: MayAlias: i8* %gep, i8* %p
+define i8 @add_no_nuw(ptr %p, i64 %n) {
+ store i8 3, ptr %p
+
+ %add = add i64 %n, 1
+ %gep = getelementptr nuw i8, ptr %p, i64 %add
+ %val = load i8, ptr %gep
+ ret i8 %val
+}
+
+; CHECK-LABEL: add_nuw
+; CHECK: NoAlias: i8* %gep, i8* %p
+define i8 @add_nuw(ptr %p, i64 %n) {
+ store i8 3, ptr %p
+
+ %add = add nuw i64 %n, 1
+ %gep = getelementptr nuw i8, ptr %p, i64 %add
+ %val = load i8, ptr %gep
+ ret i8 %val
+}
+
+; CHECK-LABEL: add_no_nuw
+; CHECK: MayAlias: i8* %gep, i16* %p
+define i8 @add_no_nuw_scale(ptr %p, i64 %n) {
+ store i16 3, ptr %p
+
+ %add = add i64 %n, 1
+ %gep = getelementptr nuw i16, ptr %p, i64 %add
+ %val = load i8, ptr %gep
+ ret i8 %val
+}
+
+; CHECK-LABEL: add_nuw
+; CHECK: NoAlias: i8* %gep, i16* %p
+define i8 @add_nuw_scale(ptr %p, i64 %n) {
+ store i16 3, ptr %p
+
+ %add = add nuw i64 %n, 1
+ %gep = getelementptr nuw i16, ptr %p, i64 %add
+ %val = load i8, ptr %gep
+ ret i8 %val
+}
+
+; CHECK-LABEL: sub_nuw
+; CHECK: MayAlias: i8* %gep, i8* %p
+define i8 @sub_nuw(ptr %p, i64 %n) {
+ store i8 3, ptr %p
+
+ %add = sub nuw i64 %n, 1
+ %gep = getelementptr nuw i8, ptr %p, i64 %add
+ %val = load i8, ptr %gep
+ ret i8 %val
+}
+
+; CHECK-LABEL: mul_no_nuw
+; CHECK: MayAlias: i8* %gep, i16* %p
+define i8 @mul_no_nuw(ptr %p, i64 %n) {
+ store i16 3, ptr %p
+
+ %add = add nuw i64 %n, 1
+ %mul = mul i64 %add, 2
+ %gep = getelementptr nuw i8, ptr %p, i64 %mul
+ %val = load i8, ptr %gep
+ ret i8 %val
+}
+
+; CHECK-LABEL: mul_nuw
+; CHECK: NoAlias: i8* %gep, i16* %p
+define i8 @mul_nuw(ptr %p, i64 %n) {
+ store i16 3, ptr %p
+
+ %add = add nuw i64 %n, 1
+ %mul = mul nuw i64 %add, 2
+ %gep = getelementptr nuw i8, ptr %p, i64 %mul
+ %val = load i8, ptr %gep
+ ret i8 %val
+}
+
+; CHECK-LABEL: shl_no_nuw
+; CHECK: MayAlias: i8* %gep, i16* %p
+define i8 @shl_no_nuw(ptr %p, i64 %n) {
+ store i16 3, ptr %p
+
+ %add = add nuw i64 %n, 1
+ %shl = shl i64 %add, 1
+ %gep = getelementptr nuw i8, ptr %p, i64 %shl
+ %val = load i8, ptr %gep
+ ret i8 %val
+}
+
+; CHECK-LABEL: shl_nuw
+; CHECK: NoAlias: i8* %gep, i16* %p
+define i8 @shl_nuw(ptr %p, i64 %n) {
+ store i16 3, ptr %p
+
+ %add = add nuw i64 %n, 1
+ %shl = shl nuw i64 %add, 1
+ %gep = getelementptr nuw i8, ptr %p, i64 %shl
+ %val = load i8, ptr %gep
+ ret i8 %val
+}
|
nikic
commented
Aug 29, 2024
| // The check for zero offset is necessary, because generally | ||
| // (X +nsw Y) *nsw Z does not imply (X *nsw Z) +nsw (Y *nsw Z). | ||
| bool NSW = IsNSW && (Other.isOne() || (MulIsNSW && Offset.isZero())); | ||
| return LinearExpression(Val, Scale * Other, Offset * Other, NSW); | ||
| bool NUW = IsNUW && (Other.isOne() || MulIsNUW); |
There was a problem hiding this comment.
Proof that mul nuw always distributes (unlike mul nsw): https://alive2.llvm.org/ce/z/MZd_JJ
nikic
commented
Aug 29, 2024
| @@ -444,20 +448,22 @@ static LinearExpression GetLinearExpression( | |||
| E = GetLinearExpression(Val.withValue(BOp->getOperand(0), false), DL, | |||
| Depth + 1, AC, DT); | |||
| E.Offset += RHS; | |||
| E.IsNUW &= NUW; | |||
There was a problem hiding this comment.
Proof for add nuw associativity: https://alive2.llvm.org/ce/z/oiNosh
And counter-proof that the current NSW code here is incorrect: https://alive2.llvm.org/ce/z/Ga8-CT
We are missing a pre-condition: https://alive2.llvm.org/ce/z/wkP_yw
But this is unrelated to this patch...
There was a problem hiding this comment.
I have trouble actually coming up with a test case that currently gets miscompiled due to the NSW issue ... ideas welcome :)
hazzlim
approved these changes
Sep 2, 2024
hazzlim
added a commit
to hazzlim/llvm-project
that referenced
this pull request
Sep 4, 2024
Add nuw attribute to inbounds GEPs where the expression used to form the GEP is an addition of unsigned indices. Relands llvm#105496, which was reverted because it exposed a miscompilation arising from llvm#98608. This is now fixed by llvm#106512.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we decompose the GEP offset expression, and the arithmetic is not performed using nuw operations, we cannot retain the nuw flag on the decomposed GEP.
For example, if we have
gep nuw p, (a-1), this is not at all the same asgep nuw (gep nuw p, a), -1.Fix this by tracking NUW through linear expression decomposition, similarly to what we already do for the NSW flag.
This fixes the miscompilation reported in #105496 (comment).