[llvm] c31eb82 - [BasicAA] Fix nsw handling for negated scales (PR63266)
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 27 00:40:16 PDT 2023
Author: Nikita Popov
Date: 2023-06-27T09:40:09+02:00
New Revision: c31eb827b72da784d7f18512a51e5396ac302d72
URL: https://github.com/llvm/llvm-project/commit/c31eb827b72da784d7f18512a51e5396ac302d72
DIFF: https://github.com/llvm/llvm-project/commit/c31eb827b72da784d7f18512a51e5396ac302d72.diff
LOG: [BasicAA] Fix nsw handling for negated scales (PR63266)
We currently preserve the nsw flag when negating scales, which is
incorrect for INT_MIN.
However, just dropping the NSW flag in this case makes BasicAA
behavior unreliable and asymmetric, because we may or may not
drop the NSW flag depending on which side gets subtracted.
Instead, leave the Scale alone and add an additional IsNegated flag,
which indicates that the whole VarIndex should be interpreted as a
subtraction. This allows us to retain the NSW flag.
When accumulating the offset range, we need to use subtraction
instead of adding for IsNegated indices. Everything else works on
the absolute value of the scale, so the negation does not matter
there.
Fixes https://github.com/llvm/llvm-project/issues/63266.
Differential Revision: https://reviews.llvm.org/D153270
Added:
Modified:
llvm/lib/Analysis/BasicAliasAnalysis.cpp
llvm/test/Analysis/BasicAA/range.ll
Removed:
################################################################################
diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index ec523748e974d..16e0e1f66524f 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -461,6 +461,17 @@ struct VariableGEPIndex {
/// True if all operations in this expression are NSW.
bool IsNSW;
+ /// True if the index should be subtracted rather than added. We don't simply
+ /// negate the Scale, to avoid losing the NSW flag: X - INT_MIN*1 may be
+ /// non-wrapping, while X + INT_MIN*(-1) wraps.
+ bool IsNegated;
+
+ bool hasNegatedScaleOf(const VariableGEPIndex &Other) const {
+ if (IsNegated == Other.IsNegated)
+ return Scale == -Other.Scale;
+ return Scale == Other.Scale;
+ }
+
void dump() const {
print(dbgs());
dbgs() << "\n";
@@ -470,7 +481,9 @@ struct VariableGEPIndex {
<< ", zextbits=" << Val.ZExtBits
<< ", sextbits=" << Val.SExtBits
<< ", truncbits=" << Val.TruncBits
- << ", scale=" << Scale << ")";
+ << ", scale=" << Scale
+ << ", nsw=" << IsNSW
+ << ", negated=" << IsNegated << ")";
}
};
}
@@ -659,7 +672,8 @@ BasicAAResult::DecomposeGEPExpression(const Value *V, const DataLayout &DL,
Scale = adjustToIndexSize(Scale, IndexSize);
if (!!Scale) {
- VariableGEPIndex Entry = {LE.Val, Scale, CxtI, LE.IsNSW};
+ VariableGEPIndex Entry = {LE.Val, Scale, CxtI, LE.IsNSW,
+ /* IsNegated */ false};
Decomposed.VarIndices.push_back(Entry);
}
}
@@ -1151,9 +1165,14 @@ AliasResult BasicAAResult::aliasGEP(
assert(OffsetRange.getBitWidth() == Scale.getBitWidth() &&
"Bit widths are normalized to MaxIndexSize");
if (Index.IsNSW)
- OffsetRange = OffsetRange.add(CR.smul_sat(ConstantRange(Scale)));
+ CR = CR.smul_sat(ConstantRange(Scale));
+ else
+ CR = CR.smul_fast(ConstantRange(Scale));
+
+ if (Index.IsNegated)
+ OffsetRange = OffsetRange.sub(CR);
else
- OffsetRange = OffsetRange.add(CR.smul_fast(ConstantRange(Scale)));
+ OffsetRange = OffsetRange.add(CR);
}
// We now have accesses at two offsets from the same base:
@@ -1220,7 +1239,7 @@ AliasResult BasicAAResult::aliasGEP(
// inequality of values across loop iterations.
const VariableGEPIndex &Var0 = DecompGEP1.VarIndices[0];
const VariableGEPIndex &Var1 = DecompGEP1.VarIndices[1];
- if (Var0.Scale == -Var1.Scale && Var0.Val.TruncBits == 0 &&
+ if (Var0.hasNegatedScaleOf(Var1) && Var0.Val.TruncBits == 0 &&
Var0.Val.hasSameCastsAs(Var1.Val) && !AAQI.MayBeCrossIteration &&
isKnownNonEqual(Var0.Val.V, Var1.Val.V, DL, &AC, /* CxtI */ nullptr,
DT))
@@ -1701,6 +1720,13 @@ void BasicAAResult::subtractDecomposedGEPs(DecomposedGEP &DestGEP,
!Dest.Val.hasSameCastsAs(Src.Val))
continue;
+ // Normalize IsNegated if we're going to lose the NSW flag anyway.
+ if (Dest.IsNegated) {
+ Dest.Scale = -Dest.Scale;
+ Dest.IsNegated = false;
+ Dest.IsNSW = false;
+ }
+
// If we found it, subtract off Scale V's from the entry in Dest. If it
// goes to zero, remove the entry.
if (Dest.Scale != Src.Scale) {
@@ -1715,7 +1741,8 @@ void BasicAAResult::subtractDecomposedGEPs(DecomposedGEP &DestGEP,
// If we didn't consume this entry, add it to the end of the Dest list.
if (!Found) {
- VariableGEPIndex Entry = {Src.Val, -Src.Scale, Src.CxtI, Src.IsNSW};
+ VariableGEPIndex Entry = {Src.Val, Src.Scale, Src.CxtI, Src.IsNSW,
+ /* IsNegated */ true};
DestGEP.VarIndices.push_back(Entry);
}
}
@@ -1737,7 +1764,7 @@ bool BasicAAResult::constantOffsetHeuristic(const DecomposedGEP &GEP,
const VariableGEPIndex &Var0 = GEP.VarIndices[0], &Var1 = GEP.VarIndices[1];
if (Var0.Val.TruncBits != 0 || !Var0.Val.hasSameCastsAs(Var1.Val) ||
- Var0.Scale != -Var1.Scale ||
+ !Var0.hasNegatedScaleOf(Var1) ||
Var0.Val.V->getType() != Var1.Val.V->getType())
return false;
diff --git a/llvm/test/Analysis/BasicAA/range.ll b/llvm/test/Analysis/BasicAA/range.ll
index 06c8103994a8a..3862e26ee49a4 100644
--- a/llvm/test/Analysis/BasicAA/range.ll
+++ b/llvm/test/Analysis/BasicAA/range.ll
@@ -239,9 +239,8 @@ define void @benign_overflow(ptr %p, i64 %o) {
ret void
}
-; FIXME: This is a miscompile
; CHECK-LABEL: pr63266
-; CHECK: NoAlias: i8* %gep2, i8* %offset16
+; CHECK: MayAlias: i8* %gep2, i8* %offset16
define void @pr63266(i1 %c, ptr %base) {
entry:
%offset16 = getelementptr inbounds i8, ptr %base, i64 16
More information about the llvm-commits
mailing list