[llvm] cc158d4 - Reapply [ConstantFold] Remove non-trivial gep-of-gep fold (#93823)
Nikita Popov via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 10 01:13:02 PDT 2024
Author: Nikita Popov
Date: 2024-06-10T10:12:55+02:00
New Revision: cc158d4c0bb50554dadcad44c10cafb052376cea
URL: https://github.com/llvm/llvm-project/commit/cc158d4c0bb50554dadcad44c10cafb052376cea
DIFF: https://github.com/llvm/llvm-project/commit/cc158d4c0bb50554dadcad44c10cafb052376cea.diff
LOG: Reapply [ConstantFold] Remove non-trivial gep-of-gep fold (#93823)
Reapply after https://github.com/llvm/llvm-project/pull/93956, which
changed clang array initialization codegen to avoid size regressions
for unoptimized builds.
-----
This fold is subtly incorrect, because DL-unaware constant folding does
not know the correct index type to use, and just performs the addition
in the type that happens to already be there. This is incorrect, since
sext(X)+sext(Y) is generally not the same as sext(X+Y). See the
`@constexpr_gep_of_gep_with_narrow_type()` for a miscompile with the
current implementation.
One could try to restrict the fold to cases where no overflow occurs,
but I'm not bothering with that here, because the DL-aware constant
folding will take care of this anyway. I've only kept the
straightforward zero-index case, where we just concatenate two GEPs.
Added:
Modified:
llvm/lib/IR/ConstantFold.cpp
llvm/test/Other/constant-fold-gep.ll
llvm/test/Transforms/InstCombine/gepgep.ll
llvm/test/Transforms/InstCombine/getelementptr.ll
llvm/test/Transforms/InstCombine/ptrtoint-nullgep.ll
Removed:
################################################################################
diff --git a/llvm/lib/IR/ConstantFold.cpp b/llvm/lib/IR/ConstantFold.cpp
index 7b8b9a7b652da..77a833610a3a9 100644
--- a/llvm/lib/IR/ConstantFold.cpp
+++ b/llvm/lib/IR/ConstantFold.cpp
@@ -1416,64 +1416,16 @@ static Constant *foldGEPOfGEP(GEPOperator *GEP, Type *PointeeTy, bool InBounds,
if (GEP->getInRange())
return nullptr;
+ // Only handle simple case with leading zero index. We cannot perform an
+ // actual addition as we don't know the correct index type size to use.
Constant *Idx0 = cast<Constant>(Idxs[0]);
- if (Idx0->isNullValue()) {
- // Handle the simple case of a zero index.
- SmallVector<Value*, 16> NewIndices;
- NewIndices.reserve(Idxs.size() + GEP->getNumIndices());
- NewIndices.append(GEP->idx_begin(), GEP->idx_end());
- NewIndices.append(Idxs.begin() + 1, Idxs.end());
- return ConstantExpr::getGetElementPtr(
- GEP->getSourceElementType(), cast<Constant>(GEP->getPointerOperand()),
- NewIndices, InBounds && GEP->isInBounds());
- }
-
- gep_type_iterator LastI = gep_type_end(GEP);
- for (gep_type_iterator I = gep_type_begin(GEP), E = gep_type_end(GEP);
- I != E; ++I)
- LastI = I;
-
- // We can't combine GEPs if the last index is a struct type.
- if (!LastI.isSequential())
- return nullptr;
- // We could perform the transform with non-constant index, but prefer leaving
- // it as GEP of GEP rather than GEP of add for now.
- ConstantInt *CI = dyn_cast<ConstantInt>(Idx0);
- if (!CI)
- return nullptr;
-
- // TODO: This code may be extended to handle vectors as well.
- auto *LastIdx = cast<Constant>(GEP->getOperand(GEP->getNumOperands()-1));
- Type *LastIdxTy = LastIdx->getType();
- if (LastIdxTy->isVectorTy())
+ if (!Idx0->isNullValue())
return nullptr;
SmallVector<Value*, 16> NewIndices;
NewIndices.reserve(Idxs.size() + GEP->getNumIndices());
- NewIndices.append(GEP->idx_begin(), GEP->idx_end() - 1);
-
- // Add the last index of the source with the first index of the new GEP.
- // Make sure to handle the case when they are actually
diff erent types.
- if (LastIdxTy != Idx0->getType()) {
- unsigned CommonExtendedWidth =
- std::max(LastIdxTy->getIntegerBitWidth(),
- Idx0->getType()->getIntegerBitWidth());
- CommonExtendedWidth = std::max(CommonExtendedWidth, 64U);
-
- Type *CommonTy =
- Type::getIntNTy(LastIdxTy->getContext(), CommonExtendedWidth);
- if (Idx0->getType() != CommonTy)
- Idx0 = ConstantFoldCastInstruction(Instruction::SExt, Idx0, CommonTy);
- if (LastIdx->getType() != CommonTy)
- LastIdx =
- ConstantFoldCastInstruction(Instruction::SExt, LastIdx, CommonTy);
- if (!Idx0 || !LastIdx)
- return nullptr;
- }
-
- NewIndices.push_back(ConstantExpr::get(Instruction::Add, Idx0, LastIdx));
+ NewIndices.append(GEP->idx_begin(), GEP->idx_end());
NewIndices.append(Idxs.begin() + 1, Idxs.end());
-
return ConstantExpr::getGetElementPtr(
GEP->getSourceElementType(), cast<Constant>(GEP->getPointerOperand()),
NewIndices, InBounds && GEP->isInBounds());
diff --git a/llvm/test/Other/constant-fold-gep.ll b/llvm/test/Other/constant-fold-gep.ll
index d4bd24df215e8..e0d535e695273 100644
--- a/llvm/test/Other/constant-fold-gep.ll
+++ b/llvm/test/Other/constant-fold-gep.ll
@@ -104,7 +104,7 @@
; Fold GEP of a GEP. Very simple cases are folded without targetdata.
-; PLAIN: @Y = global ptr getelementptr inbounds ([3 x { i32, i32 }], ptr @ext, i64 2)
+; PLAIN: @Y = global ptr getelementptr inbounds ([3 x { i32, i32 }], ptr getelementptr inbounds ([3 x { i32, i32 }], ptr @ext, i64 1), i64 1)
; PLAIN: @Z = global ptr getelementptr inbounds (i32, ptr getelementptr inbounds ([3 x { i32, i32 }], ptr @ext, i64 0, i64 1, i32 0), i64 1)
; OPT: @Y = local_unnamed_addr global ptr getelementptr inbounds (i8, ptr @ext, i64 48)
; OPT: @Z = local_unnamed_addr global ptr getelementptr inbounds (i8, ptr @ext, i64 12)
diff --git a/llvm/test/Transforms/InstCombine/gepgep.ll b/llvm/test/Transforms/InstCombine/gepgep.ll
index d2a0e1db392ba..6b6a1597d560b 100644
--- a/llvm/test/Transforms/InstCombine/gepgep.ll
+++ b/llvm/test/Transforms/InstCombine/gepgep.ll
@@ -10,7 +10,7 @@ declare void @use(ptr)
define void @f() {
; CHECK-LABEL: define void @f() {
-; CHECK-NEXT: call void @use(ptr getelementptr (i8, ptr @buffer, i64 add (i64 sub (i64 0, i64 ptrtoint (ptr @buffer to i64)), i64 127)))
+; CHECK-NEXT: call void @use(ptr getelementptr (i8, ptr getelementptr (i8, ptr @buffer, i64 add (i64 sub (i64 0, i64 ptrtoint (ptr @buffer to i64)), i64 63)), i64 64))
; CHECK-NEXT: ret void
;
call void @use(ptr getelementptr (i8, ptr getelementptr (i8, ptr @buffer, i64 add (i64 sub (i64 0, i64 ptrtoint (ptr @buffer to i64)), i64 63)), i64 64))
diff --git a/llvm/test/Transforms/InstCombine/getelementptr.ll b/llvm/test/Transforms/InstCombine/getelementptr.ll
index d5cfb74a44430..871913610429b 100644
--- a/llvm/test/Transforms/InstCombine/getelementptr.ll
+++ b/llvm/test/Transforms/InstCombine/getelementptr.ll
@@ -1716,10 +1716,9 @@ if.else:
@g = external global i8
-; FIXME: This is a miscompile
define ptr @constexpr_gep_of_gep_with_narrow_type() {
; CHECK-LABEL: @constexpr_gep_of_gep_with_narrow_type(
-; CHECK-NEXT: ret ptr getelementptr (i8, ptr @g, i64 -2)
+; CHECK-NEXT: ret ptr getelementptr (i8, ptr @g, i64 254)
;
ret ptr getelementptr (i8, ptr getelementptr (i8, ptr @g, i8 127), i8 127)
}
diff --git a/llvm/test/Transforms/InstCombine/ptrtoint-nullgep.ll b/llvm/test/Transforms/InstCombine/ptrtoint-nullgep.ll
index e6e63778d82f0..9d6f0abce84c7 100644
--- a/llvm/test/Transforms/InstCombine/ptrtoint-nullgep.ll
+++ b/llvm/test/Transforms/InstCombine/ptrtoint-nullgep.ll
@@ -68,10 +68,10 @@ define void @constant_fold_ptrtoint_of_gep_of_nullgep() {
; LLPARSER-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr inbounds (i8, ptr addrspace(1) null, i64 1234) to i64))
; LLPARSER-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr (i8, ptr addrspace(1) null, i64 1234) to i64))
; LLPARSER-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr (i8, ptr addrspace(1) null, i64 1234) to i64))
-; LLPARSER-NEXT: call void @use_i64(i64 0)
-; LLPARSER-NEXT: call void @use_i64(i64 0)
-; LLPARSER-NEXT: call void @use_i64(i64 0)
-; LLPARSER-NEXT: call void @use_i64(i64 0)
+; LLPARSER-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr inbounds (i8, ptr addrspace(1) getelementptr inbounds (i8, ptr addrspace(1) null, i64 -1), i64 1) to i64))
+; LLPARSER-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr (i8, ptr addrspace(1) getelementptr inbounds (i8, ptr addrspace(1) null, i64 -1), i64 1) to i64))
+; LLPARSER-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr inbounds (i8, ptr addrspace(1) getelementptr (i8, ptr addrspace(1) null, i64 -1), i64 1) to i64))
+; LLPARSER-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr (i8, ptr addrspace(1) getelementptr (i8, ptr addrspace(1) null, i64 -1), i64 1) to i64))
; LLPARSER-NEXT: ret void
;
; INSTSIMPLIFY-LABEL: define {{[^@]+}}@constant_fold_ptrtoint_of_gep_of_nullgep() {
@@ -83,10 +83,10 @@ define void @constant_fold_ptrtoint_of_gep_of_nullgep() {
; INSTSIMPLIFY-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr inbounds (i8, ptr addrspace(1) null, i64 1234) to i64))
; INSTSIMPLIFY-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr (i8, ptr addrspace(1) null, i64 1234) to i64))
; INSTSIMPLIFY-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr (i8, ptr addrspace(1) null, i64 1234) to i64))
-; INSTSIMPLIFY-NEXT: call void @use_i64(i64 0)
-; INSTSIMPLIFY-NEXT: call void @use_i64(i64 0)
-; INSTSIMPLIFY-NEXT: call void @use_i64(i64 0)
-; INSTSIMPLIFY-NEXT: call void @use_i64(i64 0)
+; INSTSIMPLIFY-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr inbounds (i8, ptr addrspace(1) getelementptr inbounds (i8, ptr addrspace(1) null, i64 -1), i64 1) to i64))
+; INSTSIMPLIFY-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr (i8, ptr addrspace(1) getelementptr inbounds (i8, ptr addrspace(1) null, i64 -1), i64 1) to i64))
+; INSTSIMPLIFY-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr inbounds (i8, ptr addrspace(1) getelementptr (i8, ptr addrspace(1) null, i64 -1), i64 1) to i64))
+; INSTSIMPLIFY-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr (i8, ptr addrspace(1) getelementptr (i8, ptr addrspace(1) null, i64 -1), i64 1) to i64))
; INSTSIMPLIFY-NEXT: ret void
;
; INSTCOMBINE-LABEL: define {{[^@]+}}@constant_fold_ptrtoint_of_gep_of_nullgep() {
More information about the llvm-commits
mailing list