[llvm] [NaryReassociate] Fix crash from pointer width / index width confusion (PR #125923)
Krzysztof Drewniak via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 6 09:34:52 PST 2025
https://github.com/krzysz00 updated https://github.com/llvm/llvm-project/pull/125923
>From 68c9124c7024a952c1806b8c021c4204f431033a Mon Sep 17 00:00:00 2001
From: Krzysztof Drewniak <Krzysztof.Drewniak at amd.com>
Date: Wed, 5 Feb 2025 20:00:55 +0000
Subject: [PATCH 1/3] [NaryReassociate] Fix crash from pointer width / index
width confusion
NaryReassociate would crash on expressions like the one in the added
test that involved pointers where the size of the type was greater
than the index width of the pointer, causing calls to SCEV's zext
expression on types that didn't need to be zero-extended.
This commit fixes the issue.
---
llvm/lib/Transforms/Scalar/NaryReassociate.cpp | 14 +++++++++-----
llvm/test/Transforms/NaryReassociate/nary-gep.ll | 11 +++++++++++
2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/llvm/lib/Transforms/Scalar/NaryReassociate.cpp b/llvm/lib/Transforms/Scalar/NaryReassociate.cpp
index 39720672c202d7f..434451abbb84cbc 100644
--- a/llvm/lib/Transforms/Scalar/NaryReassociate.cpp
+++ b/llvm/lib/Transforms/Scalar/NaryReassociate.cpp
@@ -402,16 +402,20 @@ NaryReassociatePass::tryReassociateGEPAtIndex(GetElementPtrInst *GEP,
IndexExprs.push_back(SE->getSCEV(Index));
// Replace the I-th index with LHS.
IndexExprs[I] = SE->getSCEV(LHS);
+ Type *GEPArgType = GEP->getOperand(I)->getType();
+ Type *LHSType = LHS->getType();
+ size_t LHSSize = DL->getTypeSizeInBits(LHSType).getFixedValue();
+ size_t GEPArgSize = DL->getTypeSizeInBits(GEPArgType).getFixedValue();
+ // For pointers, we need to look at the index size, not the total type size.
+ if (isa<PointerType>(GEPArgType))
+ GEPArgSize = DL->getIndexTypeSizeInBits(GEPArgType);
if (isKnownNonNegative(LHS, SimplifyQuery(*DL, DT, AC, GEP)) &&
- DL->getTypeSizeInBits(LHS->getType()).getFixedValue() <
- DL->getTypeSizeInBits(GEP->getOperand(I)->getType())
- .getFixedValue()) {
+ LHSSize < GEPArgSize) {
// Zero-extend LHS if it is non-negative. InstCombine canonicalizes sext to
// zext if the source operand is proved non-negative. We should do that
// consistently so that CandidateExpr more likely appears before. See
// @reassociate_gep_assume for an example of this canonicalization.
- IndexExprs[I] =
- SE->getZeroExtendExpr(IndexExprs[I], GEP->getOperand(I)->getType());
+ IndexExprs[I] = SE->getZeroExtendExpr(IndexExprs[I], GEPArgType);
}
const SCEV *CandidateExpr = SE->getGEPExpr(cast<GEPOperator>(GEP),
IndexExprs);
diff --git a/llvm/test/Transforms/NaryReassociate/nary-gep.ll b/llvm/test/Transforms/NaryReassociate/nary-gep.ll
index d0ece1e11de5a3c..a2cd32f16b88ae6 100644
--- a/llvm/test/Transforms/NaryReassociate/nary-gep.ll
+++ b/llvm/test/Transforms/NaryReassociate/nary-gep.ll
@@ -21,6 +21,17 @@ define void @no_sext_fat_pointer(ptr addrspace(2) %a, i32 %i, i32 %j) {
ret void
}
+define ptr addrspace(2) @zext_fat_pointer_crash() {
+; CHECK-LABEL: @zext_fat_pointer_crash(
+; CHECK-NEXT: [[C:%.*]] = add i32 0, 0
+; CHECK-NEXT: [[Q:%.*]] = getelementptr double, ptr addrspace(2) null, i32 [[C]]
+; CHECK-NEXT: ret ptr addrspace(2) [[Q]]
+;
+ %c = add i32 0, 0
+ %q = getelementptr double, ptr addrspace(2) null, i32 %c
+ ret ptr addrspace(2) %q
+}
+
define void @or_disjoint(ptr addrspace(2) %a, i32 %i, i32 %j, i32 %k) {
; CHECK-LABEL: @or_disjoint(
; CHECK-NEXT: [[OR:%.*]] = or disjoint i32 [[I:%.*]], [[J:%.*]]
>From cf2867ad8f54e9d83c7838806dc14c6ec8834818 Mon Sep 17 00:00:00 2001
From: Krzysztof Drewniak <Krzysztof.Drewniak at amd.com>
Date: Wed, 5 Feb 2025 23:09:05 +0000
Subject: [PATCH 2/3] Review feedback
---
llvm/lib/Transforms/Scalar/NaryReassociate.cpp | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/llvm/lib/Transforms/Scalar/NaryReassociate.cpp b/llvm/lib/Transforms/Scalar/NaryReassociate.cpp
index 434451abbb84cbc..3b3f32a0ea591fb 100644
--- a/llvm/lib/Transforms/Scalar/NaryReassociate.cpp
+++ b/llvm/lib/Transforms/Scalar/NaryReassociate.cpp
@@ -402,13 +402,10 @@ NaryReassociatePass::tryReassociateGEPAtIndex(GetElementPtrInst *GEP,
IndexExprs.push_back(SE->getSCEV(Index));
// Replace the I-th index with LHS.
IndexExprs[I] = SE->getSCEV(LHS);
- Type *GEPArgType = GEP->getOperand(I)->getType();
- Type *LHSType = LHS->getType();
+ Type *GEPArgType = SE->getEffectiveSCEVType(GEP->getOperand(I)->getType());
+ Type *LHSType = SE->getEffectiveSCEVType(LHS->getType());
size_t LHSSize = DL->getTypeSizeInBits(LHSType).getFixedValue();
size_t GEPArgSize = DL->getTypeSizeInBits(GEPArgType).getFixedValue();
- // For pointers, we need to look at the index size, not the total type size.
- if (isa<PointerType>(GEPArgType))
- GEPArgSize = DL->getIndexTypeSizeInBits(GEPArgType);
if (isKnownNonNegative(LHS, SimplifyQuery(*DL, DT, AC, GEP)) &&
LHSSize < GEPArgSize) {
// Zero-extend LHS if it is non-negative. InstCombine canonicalizes sext to
>From 1c279a6795718f4bae3259afdc99ba94a8afe192 Mon Sep 17 00:00:00 2001
From: Krzysztof Drewniak <Krzysztof.Drewniak at amd.com>
Date: Thu, 6 Feb 2025 17:34:39 +0000
Subject: [PATCH 3/3] Make the test less trivial
---
llvm/test/Transforms/NaryReassociate/nary-gep.ll | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/llvm/test/Transforms/NaryReassociate/nary-gep.ll b/llvm/test/Transforms/NaryReassociate/nary-gep.ll
index a2cd32f16b88ae6..b56fdbe4a0669d4 100644
--- a/llvm/test/Transforms/NaryReassociate/nary-gep.ll
+++ b/llvm/test/Transforms/NaryReassociate/nary-gep.ll
@@ -21,14 +21,14 @@ define void @no_sext_fat_pointer(ptr addrspace(2) %a, i32 %i, i32 %j) {
ret void
}
-define ptr addrspace(2) @zext_fat_pointer_crash() {
+define ptr addrspace(2) @zext_fat_pointer_crash(ptr addrspace(2) %p, i32 %a) {
; CHECK-LABEL: @zext_fat_pointer_crash(
-; CHECK-NEXT: [[C:%.*]] = add i32 0, 0
-; CHECK-NEXT: [[Q:%.*]] = getelementptr double, ptr addrspace(2) null, i32 [[C]]
+; CHECK-NEXT: [[C:%.*]] = add i32 [[A:%.*]], 1
+; CHECK-NEXT: [[Q:%.*]] = getelementptr double, ptr addrspace(2) [[P:%.*]], i32 [[C]]
; CHECK-NEXT: ret ptr addrspace(2) [[Q]]
;
- %c = add i32 0, 0
- %q = getelementptr double, ptr addrspace(2) null, i32 %c
+ %c = add i32 %a, 1
+ %q = getelementptr double, ptr addrspace(2) %p, i32 %c
ret ptr addrspace(2) %q
}
More information about the llvm-commits
mailing list