Skip to content

Commit 916e8f7

Browse files
kasuga-fjkasuga-fj
authored
[DA] Check nsw when extracting a constant operand of SCEVMul (llvm#164408)
Given a `SCEVMulExpr` such as `5 * %m`, `gcdMIVtest` in DA assumes the value as a multiple of 5 in a mathematical sense. However, this is not necessarily true if `5 * %m` overflows, especially because an odd number has an inverse modulo `2^64`. Such incorrect assumptions can lead to invalid analysis results. This patch stops unconditionally extracting a constant operand from `SCEVMulExpr`. Instead, it only allows this when the `SCEVMulExpr` has the `nsw` flag.
1 parent cc1022c commit 916e8f7

File tree

5 files changed

+25
-24
lines changed

5 files changed

+25
-24
lines changed

‎llvm/lib/Analysis/DependenceAnalysis.cpp‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2854,14 +2854,18 @@ bool DependenceInfo::testMIV(const SCEV *Src, const SCEV *Dst,
28542854
banerjeeMIVtest(Src, Dst, Loops, Result);
28552855
}
28562856

2857-
// Given a product, e.g., 10*X*Y, returns the first constant operand,
2858-
// in this case 10. If there is no constant part, returns std::nullopt.
2859-
static std::optional<APInt> getConstantPart(const SCEV *Expr) {
2857+
/// Given a SCEVMulExpr, returns its first operand if its first operand is a
2858+
/// constant and the product doesn't overflow in a signed sense. Otherwise,
2859+
/// returns std::nullopt. For example, given (10 * X * Y)<nsw>, it returns 10.
2860+
/// Notably, if it doesn't have nsw, the multiplication may overflow, and if
2861+
/// so, it may not a multiple of 10.
2862+
static std::optional<APInt> getConstanCoefficient(const SCEV *Expr) {
28602863
if (const auto *Constant = dyn_cast<SCEVConstant>(Expr))
28612864
return Constant->getAPInt();
28622865
if (const auto *Product = dyn_cast<SCEVMulExpr>(Expr))
28632866
if (const auto *Constant = dyn_cast<SCEVConstant>(Product->getOperand(0)))
2864-
return Constant->getAPInt();
2867+
if (Product->hasNoSignedWrap())
2868+
return Constant->getAPInt();
28652869
return std::nullopt;
28662870
}
28672871

@@ -2887,7 +2891,7 @@ bool DependenceInfo::accumulateCoefficientsGCD(const SCEV *Expr,
28872891
if (AddRec->getLoop() == CurLoop) {
28882892
CurLoopCoeff = Step;
28892893
} else {
2890-
std::optional<APInt> ConstCoeff = getConstantPart(Step);
2894+
std::optional<APInt> ConstCoeff = getConstanCoefficient(Step);
28912895

28922896
// If the coefficient is the product of a constant and other stuff, we can
28932897
// use the constant in the GCD computation.
@@ -2940,7 +2944,7 @@ bool DependenceInfo::gcdMIVtest(const SCEV *Src, const SCEV *Dst,
29402944
const SCEV *Coeff = AddRec->getStepRecurrence(*SE);
29412945
// If the coefficient is the product of a constant and other stuff,
29422946
// we can use the constant in the GCD computation.
2943-
std::optional<APInt> ConstCoeff = getConstantPart(Coeff);
2947+
std::optional<APInt> ConstCoeff = getConstanCoefficient(Coeff);
29442948
if (!ConstCoeff)
29452949
return false;
29462950
RunningGCD = APIntOps::GreatestCommonDivisor(RunningGCD, ConstCoeff->abs());
@@ -2958,7 +2962,7 @@ bool DependenceInfo::gcdMIVtest(const SCEV *Src, const SCEV *Dst,
29582962
const SCEV *Coeff = AddRec->getStepRecurrence(*SE);
29592963
// If the coefficient is the product of a constant and other stuff,
29602964
// we can use the constant in the GCD computation.
2961-
std::optional<APInt> ConstCoeff = getConstantPart(Coeff);
2965+
std::optional<APInt> ConstCoeff = getConstanCoefficient(Coeff);
29622966
if (!ConstCoeff)
29632967
return false;
29642968
RunningGCD = APIntOps::GreatestCommonDivisor(RunningGCD, ConstCoeff->abs());
@@ -2979,7 +2983,7 @@ bool DependenceInfo::gcdMIVtest(const SCEV *Src, const SCEV *Dst,
29792983
} else if (const SCEVMulExpr *Product = dyn_cast<SCEVMulExpr>(Operand)) {
29802984
// Search for constant operand to participate in GCD;
29812985
// If none found; return false.
2982-
std::optional<APInt> ConstOp = getConstantPart(Product);
2986+
std::optional<APInt> ConstOp = getConstanCoefficient(Product);
29832987
if (!ConstOp)
29842988
return false;
29852989
ExtraGCD = APIntOps::GreatestCommonDivisor(ExtraGCD, ConstOp->abs());
@@ -3032,7 +3036,7 @@ bool DependenceInfo::gcdMIVtest(const SCEV *Src, const SCEV *Dst,
30323036
Delta = SE->getMinusSCEV(SrcCoeff, DstCoeff);
30333037
// If the coefficient is the product of a constant and other stuff,
30343038
// we can use the constant in the GCD computation.
3035-
std::optional<APInt> ConstCoeff = getConstantPart(Delta);
3039+
std::optional<APInt> ConstCoeff = getConstanCoefficient(Delta);
30363040
if (!ConstCoeff)
30373041
// The difference of the two coefficients might not be a product
30383042
// or constant, in which case we give up on this direction.

‎llvm/test/Analysis/DependenceAnalysis/GCD.ll‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,7 @@ define void @gcd4(ptr %A, ptr %B, i64 %M, i64 %N) nounwind uwtable ssp {
254254
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: store i32 %conv, ptr %arrayidx, align 4
255255
; CHECK-NEXT: da analyze - output [* *]!
256256
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: %0 = load i32, ptr %arrayidx16, align 4
257-
; CHECK-NEXT: da analyze - none!
257+
; CHECK-NEXT: da analyze - flow [* *|<]!
258258
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: store i32 %0, ptr %B.addr.11, align 4
259259
; CHECK-NEXT: da analyze - confused!
260260
; CHECK-NEXT: Src: %0 = load i32, ptr %arrayidx16, align 4 --> Dst: %0 = load i32, ptr %arrayidx16, align 4
@@ -322,7 +322,7 @@ define void @gcd5(ptr %A, ptr %B, i64 %M, i64 %N) nounwind uwtable ssp {
322322
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: store i32 %conv, ptr %arrayidx, align 4
323323
; CHECK-NEXT: da analyze - output [* *]!
324324
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: %0 = load i32, ptr %arrayidx16, align 4
325-
; CHECK-NEXT: da analyze - flow [<> *]!
325+
; CHECK-NEXT: da analyze - flow [* *|<]!
326326
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: store i32 %0, ptr %B.addr.11, align 4
327327
; CHECK-NEXT: da analyze - confused!
328328
; CHECK-NEXT: Src: %0 = load i32, ptr %arrayidx16, align 4 --> Dst: %0 = load i32, ptr %arrayidx16, align 4
@@ -390,7 +390,7 @@ define void @gcd6(i64 %n, ptr %A, ptr %B) nounwind uwtable ssp {
390390
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx5, align 4 --> Dst: store i32 %conv, ptr %arrayidx5, align 4
391391
; CHECK-NEXT: da analyze - output [* *]!
392392
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx5, align 4 --> Dst: %2 = load i32, ptr %arrayidx9, align 4
393-
; CHECK-NEXT: da analyze - none!
393+
; CHECK-NEXT: da analyze - flow [* *|<]!
394394
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx5, align 4 --> Dst: store i32 %2, ptr %B.addr.12, align 4
395395
; CHECK-NEXT: da analyze - confused!
396396
; CHECK-NEXT: Src: %2 = load i32, ptr %arrayidx9, align 4 --> Dst: %2 = load i32, ptr %arrayidx9, align 4

‎llvm/test/Analysis/DependenceAnalysis/SymbolicSIV.ll‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -384,7 +384,7 @@ define void @symbolicsiv6(ptr %A, ptr %B, i64 %n, i64 %N, i64 %M) nounwind uwtab
384384
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: store i32 %conv, ptr %arrayidx, align 4
385385
; CHECK-NEXT: da analyze - none!
386386
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: %0 = load i32, ptr %arrayidx7, align 4
387-
; CHECK-NEXT: da analyze - none!
387+
; CHECK-NEXT: da analyze - flow [*|<]!
388388
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: store i32 %0, ptr %B.addr.02, align 4
389389
; CHECK-NEXT: da analyze - confused!
390390
; CHECK-NEXT: Src: %0 = load i32, ptr %arrayidx7, align 4 --> Dst: %0 = load i32, ptr %arrayidx7, align 4
@@ -440,7 +440,7 @@ define void @symbolicsiv7(ptr %A, ptr %B, i64 %n, i64 %N, i64 %M) nounwind uwtab
440440
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: store i32 %conv, ptr %arrayidx, align 4
441441
; CHECK-NEXT: da analyze - none!
442442
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: %1 = load i32, ptr %arrayidx6, align 4
443-
; CHECK-NEXT: da analyze - flow [<>]!
443+
; CHECK-NEXT: da analyze - flow [*|<]!
444444
; CHECK-NEXT: Src: store i32 %conv, ptr %arrayidx, align 4 --> Dst: store i32 %1, ptr %B.addr.02, align 4
445445
; CHECK-NEXT: da analyze - confused!
446446
; CHECK-NEXT: Src: %1 = load i32, ptr %arrayidx6, align 4 --> Dst: %1 = load i32, ptr %arrayidx6, align 4

‎llvm/test/Analysis/DependenceAnalysis/compute-absolute-value.ll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ define void @unknown_sign(ptr %a, i64 %k) {
1818
; CHECK-NEXT: Src: store i8 1, ptr %idx.0, align 1 --> Dst: store i8 1, ptr %idx.0, align 1
1919
; CHECK-NEXT: da analyze - none!
2020
; CHECK-NEXT: Src: store i8 1, ptr %idx.0, align 1 --> Dst: store i8 2, ptr %idx.1, align 1
21-
; CHECK-NEXT: da analyze - output [<>]!
21+
; CHECK-NEXT: da analyze - output [*|<]!
2222
; CHECK-NEXT: Src: store i8 2, ptr %idx.1, align 1 --> Dst: store i8 2, ptr %idx.1, align 1
2323
; CHECK-NEXT: da analyze - none!
2424
;

‎llvm/test/Analysis/DependenceAnalysis/gcd-miv-overflow.ll‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -13,31 +13,28 @@
1313
; offset1 += 3;
1414
; }
1515
;
16-
; FIXME: DependenceAnalysis currently detects no dependency between the two
17-
; stores, but it does exist. E.g., consider `m` is 12297829382473034411, which
18-
; is a modular multiplicative inverse of 3 under modulo 2^64. Then `offset0` is
19-
; effectively `i + 4`, so accesses will be as follows:
16+
; Dependency exists between the two stores. E.g., consider `m` is
17+
; 12297829382473034411, which is a modular multiplicative inverse of 3 under
18+
; modulo 2^64. Then `offset0` is effectively `i + 4`, so accesses will be as
19+
; follows:
2020
;
2121
; - A[offset0] : A[4], A[5], A[6], ...
2222
; - A[offset1] : A[0], A[3], A[6], ...
2323
;
24-
; The root cause is that DA interprets `3*m` in non-modular arithmetic, which
25-
; isn't necessarily true due to overflow.
26-
;
2724
define void @gcdmiv_coef_ovfl(ptr %A, i64 %m) {
2825
; CHECK-ALL-LABEL: 'gcdmiv_coef_ovfl'
2926
; CHECK-ALL-NEXT: Src: store i8 1, ptr %gep.0, align 1 --> Dst: store i8 1, ptr %gep.0, align 1
3027
; CHECK-ALL-NEXT: da analyze - none!
3128
; CHECK-ALL-NEXT: Src: store i8 1, ptr %gep.0, align 1 --> Dst: store i8 2, ptr %gep.1, align 1
32-
; CHECK-ALL-NEXT: da analyze - none!
29+
; CHECK-ALL-NEXT: da analyze - output [*|<]!
3330
; CHECK-ALL-NEXT: Src: store i8 2, ptr %gep.1, align 1 --> Dst: store i8 2, ptr %gep.1, align 1
3431
; CHECK-ALL-NEXT: da analyze - none!
3532
;
3633
; CHECK-GCD-MIV-LABEL: 'gcdmiv_coef_ovfl'
3734
; CHECK-GCD-MIV-NEXT: Src: store i8 1, ptr %gep.0, align 1 --> Dst: store i8 1, ptr %gep.0, align 1
3835
; CHECK-GCD-MIV-NEXT: da analyze - consistent output [*]!
3936
; CHECK-GCD-MIV-NEXT: Src: store i8 1, ptr %gep.0, align 1 --> Dst: store i8 2, ptr %gep.1, align 1
40-
; CHECK-GCD-MIV-NEXT: da analyze - none!
37+
; CHECK-GCD-MIV-NEXT: da analyze - consistent output [*|<]!
4138
; CHECK-GCD-MIV-NEXT: Src: store i8 2, ptr %gep.1, align 1 --> Dst: store i8 2, ptr %gep.1, align 1
4239
; CHECK-GCD-MIV-NEXT: da analyze - consistent output [*]!
4340
;

0 commit comments

Comments
 (0)