Skip to content

Commit e1cc9e4

Browse files
nikicnikic
authored
[ConstantFold] Remove non-trivial gep-of-gep fold (#93823)
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.
1 parent 63dc31b commit e1cc9e4

9 files changed

+71
-120
lines changed

‎clang/test/CodeGenCXX/2011-12-19-init-list-ctor.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -22,5 +22,5 @@ struct S {
2222
// CHECK: call void @_ZN1AC1EPKc(ptr {{[^,]*}} getelementptr inbounds (%struct.S, ptr @arr, i32 0, i32 1), ptr noundef @.str)
2323
// CHECK: store i32 1, ptr getelementptr inbounds (%struct.S, ptr @arr, i64 1)
2424
// CHECK: call void @_ZN1AC1EPKc(ptr {{[^,]*}} getelementptr inbounds (%struct.S, ptr @arr, i64 1, i32 1), ptr noundef @.str.1)
25-
// CHECK: store i32 2, ptr getelementptr inbounds (%struct.S, ptr @arr, i64 2)
26-
// CHECK: call void @_ZN1AC1EPKc(ptr {{[^,]*}} getelementptr inbounds (%struct.S, ptr @arr, i64 2, i32 1), ptr noundef @.str.2)
25+
// CHECK: store i32 2, ptr getelementptr inbounds (%struct.S, ptr getelementptr inbounds (%struct.S, ptr @arr, i64 1), i64 1)
26+
// CHECK: call void @_ZN1AC1EPKc(ptr {{[^,]*}} getelementptr inbounds (%struct.S, ptr getelementptr inbounds (%struct.S, ptr @arr, i64 1), i64 1, i32 1), ptr noundef @.str.2)

‎clang/test/CodeGenCXX/cxx0x-initializer-stdinitializerlist-pr12086.cpp

+4-4
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,8 @@ std::initializer_list<std::initializer_list<int>> nested = {
8383
// CHECK-DYNAMIC-BL: store i32 5, ptr @_ZGR6nested2_
8484
// CHECK-DYNAMIC-BL: store i32 {{.*}}, ptr getelementptr inbounds (i32, ptr @_ZGR6nested2_, i64 1)
8585
// CHECK-DYNAMIC-BL: store ptr @_ZGR6nested2_,
86-
// CHECK-DYNAMIC-BL: ptr getelementptr inbounds ({{.*}}, ptr @_ZGR6nested_, i64 2), align 8
87-
// CHECK-DYNAMIC-BL: store i64 2, ptr getelementptr inbounds ({{.*}}, ptr @_ZGR6nested_, i64 2, i32 1), align 8
86+
// CHECK-DYNAMIC-BL: ptr getelementptr inbounds ({{.*}}, ptr getelementptr inbounds ({{.*}}, ptr @_ZGR6nested_, i64 1), i64 1), align 8
87+
// CHECK-DYNAMIC-BL: store i64 2, ptr getelementptr inbounds ({{.*}}, ptr getelementptr inbounds ({{.*}}, ptr @_ZGR6nested_, i64 1), i64 1, i32 1), align 8
8888
// CHECK-DYNAMIC-BL: store ptr @_ZGR6nested_,
8989
// CHECK-DYNAMIC-BL: ptr @nested, align 8
9090
// CHECK-DYNAMIC-BL: store i64 3, ptr getelementptr inbounds ({{.*}}, ptr @nested, i32 0, i32 1), align 8
@@ -123,9 +123,9 @@ std::initializer_list<std::initializer_list<int>> nested = {
123123
// CHECK-DYNAMIC-BE: store i32 5, ptr @_ZGR6nested2_
124124
// CHECK-DYNAMIC-BE: store i32 {{.*}}, ptr getelementptr inbounds (i32, ptr @_ZGR6nested2_, i64 1)
125125
// CHECK-DYNAMIC-BE: store ptr @_ZGR6nested2_,
126-
// CHECK-DYNAMIC-BE: ptr getelementptr inbounds ({{.*}}, ptr @_ZGR6nested_, i64 2), align 8
126+
// CHECK-DYNAMIC-BE: ptr getelementptr inbounds ({{.*}}, ptr getelementptr inbounds ({{.*}}, ptr @_ZGR6nested_, i64 1), i64 1), align 8
127127
// CHECK-DYNAMIC-BE: store ptr getelementptr inbounds ([2 x i32], ptr @_ZGR6nested2_, i64 0, i64 2),
128-
// CHECK-DYNAMIC-BE: ptr getelementptr inbounds ({{.*}}, ptr @_ZGR6nested_, i64 2, i32 1), align 8
128+
// CHECK-DYNAMIC-BE: ptr getelementptr inbounds ({{.*}}, ptr getelementptr inbounds ({{.*}}, ptr @_ZGR6nested_, i64 1), i64 1, i32 1), align 8
129129
// CHECK-DYNAMIC-BE: store ptr @_ZGR6nested_,
130130
// CHECK-DYNAMIC-BE: ptr @nested, align 8
131131
// CHECK-DYNAMIC-BE: store ptr getelementptr inbounds ([3 x {{.*}}], ptr @_ZGR6nested_, i64 0, i64 3),

‎clang/test/CodeGenCXX/cxx0x-initializer-stdinitializerlist.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -370,8 +370,8 @@ namespace partly_constant {
370370
//
371371
// Third init list.
372372
// CHECK-NOT: @[[PARTLY_CONSTANT_THIRD]],
373-
// CHECK: store ptr {{.*}}@[[PARTLY_CONSTANT_THIRD]]{{.*}}, ptr getelementptr inbounds ({{.*}}, ptr {{.*}}@[[PARTLY_CONSTANT_INNER]]{{.*}}, i64 2)
374-
// CHECK: store i64 4, ptr getelementptr inbounds ({{.*}}, ptr {{.*}}@[[PARTLY_CONSTANT_INNER]]{{.*}}, i64 2, i32 1)
373+
// CHECK: store ptr {{.*}}@[[PARTLY_CONSTANT_THIRD]]{{.*}}, ptr getelementptr inbounds ({{.*}}, ptr getelementptr inbounds ({{.*}}, ptr {{.*}}@[[PARTLY_CONSTANT_INNER]]{{.*}}, i64 1), i64 1)
374+
// CHECK: store i64 4, ptr getelementptr inbounds ({{.*}}, ptr getelementptr inbounds ({{.*}}, ptr {{.*}}@[[PARTLY_CONSTANT_INNER]]{{.*}}, i64 1), i64 1, i32 1)
375375
// CHECK-NOT: @[[PARTLY_CONSTANT_THIRD]],
376376
//
377377
// Outer init list.

‎clang/test/OpenMP/threadprivate_codegen.cpp

+48-48
Large diffs are not rendered by default.

‎llvm/lib/IR/ConstantFold.cpp

+4-52
Original file line numberDiff line numberDiff line change
@@ -1432,64 +1432,16 @@ static Constant *foldGEPOfGEP(GEPOperator *GEP, Type *PointeeTy, bool InBounds,
14321432
if (GEP->getInRange())
14331433
return nullptr;
14341434

1435+
// Only handle simple case with leading zero index. We cannot perform an
1436+
// actual addition as we don't know the correct index type size to use.
14351437
Constant *Idx0 = cast<Constant>(Idxs[0]);
1436-
if (Idx0->isNullValue()) {
1437-
// Handle the simple case of a zero index.
1438-
SmallVector<Value*, 16> NewIndices;
1439-
NewIndices.reserve(Idxs.size() + GEP->getNumIndices());
1440-
NewIndices.append(GEP->idx_begin(), GEP->idx_end());
1441-
NewIndices.append(Idxs.begin() + 1, Idxs.end());
1442-
return ConstantExpr::getGetElementPtr(
1443-
GEP->getSourceElementType(), cast<Constant>(GEP->getPointerOperand()),
1444-
NewIndices, InBounds && GEP->isInBounds());
1445-
}
1446-
1447-
gep_type_iterator LastI = gep_type_end(GEP);
1448-
for (gep_type_iterator I = gep_type_begin(GEP), E = gep_type_end(GEP);
1449-
I != E; ++I)
1450-
LastI = I;
1451-
1452-
// We can't combine GEPs if the last index is a struct type.
1453-
if (!LastI.isSequential())
1454-
return nullptr;
1455-
// We could perform the transform with non-constant index, but prefer leaving
1456-
// it as GEP of GEP rather than GEP of add for now.
1457-
ConstantInt *CI = dyn_cast<ConstantInt>(Idx0);
1458-
if (!CI)
1459-
return nullptr;
1460-
1461-
// TODO: This code may be extended to handle vectors as well.
1462-
auto *LastIdx = cast<Constant>(GEP->getOperand(GEP->getNumOperands()-1));
1463-
Type *LastIdxTy = LastIdx->getType();
1464-
if (LastIdxTy->isVectorTy())
1438+
if (!Idx0->isNullValue())
14651439
return nullptr;
14661440

14671441
SmallVector<Value*, 16> NewIndices;
14681442
NewIndices.reserve(Idxs.size() + GEP->getNumIndices());
1469-
NewIndices.append(GEP->idx_begin(), GEP->idx_end() - 1);
1470-
1471-
// Add the last index of the source with the first index of the new GEP.
1472-
// Make sure to handle the case when they are actually different types.
1473-
if (LastIdxTy != Idx0->getType()) {
1474-
unsigned CommonExtendedWidth =
1475-
std::max(LastIdxTy->getIntegerBitWidth(),
1476-
Idx0->getType()->getIntegerBitWidth());
1477-
CommonExtendedWidth = std::max(CommonExtendedWidth, 64U);
1478-
1479-
Type *CommonTy =
1480-
Type::getIntNTy(LastIdxTy->getContext(), CommonExtendedWidth);
1481-
if (Idx0->getType() != CommonTy)
1482-
Idx0 = ConstantFoldCastInstruction(Instruction::SExt, Idx0, CommonTy);
1483-
if (LastIdx->getType() != CommonTy)
1484-
LastIdx =
1485-
ConstantFoldCastInstruction(Instruction::SExt, LastIdx, CommonTy);
1486-
if (!Idx0 || !LastIdx)
1487-
return nullptr;
1488-
}
1489-
1490-
NewIndices.push_back(ConstantExpr::get(Instruction::Add, Idx0, LastIdx));
1443+
NewIndices.append(GEP->idx_begin(), GEP->idx_end());
14911444
NewIndices.append(Idxs.begin() + 1, Idxs.end());
1492-
14931445
return ConstantExpr::getGetElementPtr(
14941446
GEP->getSourceElementType(), cast<Constant>(GEP->getPointerOperand()),
14951447
NewIndices, InBounds && GEP->isInBounds());

‎llvm/test/Other/constant-fold-gep.ll

+1-1
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@
104104

105105
; Fold GEP of a GEP. Very simple cases are folded without targetdata.
106106

107-
; PLAIN: @Y = global ptr getelementptr inbounds ([3 x { i32, i32 }], ptr @ext, i64 2)
107+
; PLAIN: @Y = global ptr getelementptr inbounds ([3 x { i32, i32 }], ptr getelementptr inbounds ([3 x { i32, i32 }], ptr @ext, i64 1), i64 1)
108108
; PLAIN: @Z = global ptr getelementptr inbounds (i32, ptr getelementptr inbounds ([3 x { i32, i32 }], ptr @ext, i64 0, i64 1, i32 0), i64 1)
109109
; OPT: @Y = local_unnamed_addr global ptr getelementptr inbounds (i8, ptr @ext, i64 48)
110110
; OPT: @Z = local_unnamed_addr global ptr getelementptr inbounds (i8, ptr @ext, i64 12)

‎llvm/test/Transforms/InstCombine/gepgep.ll

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ declare void @use(ptr)
1010

1111
define void @f() {
1212
; CHECK-LABEL: define void @f() {
13-
; CHECK-NEXT: call void @use(ptr getelementptr (i8, ptr @buffer, i64 add (i64 sub (i64 0, i64 ptrtoint (ptr @buffer to i64)), i64 127)))
13+
; 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))
1414
; CHECK-NEXT: ret void
1515
;
1616
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))

‎llvm/test/Transforms/InstCombine/getelementptr.ll

+1-2
Original file line numberDiff line numberDiff line change
@@ -1684,10 +1684,9 @@ if.else:
16841684

16851685
@g = external global i8
16861686

1687-
; FIXME: This is a miscompile
16881687
define ptr @constexpr_gep_of_gep_with_narrow_type() {
16891688
; CHECK-LABEL: @constexpr_gep_of_gep_with_narrow_type(
1690-
; CHECK-NEXT: ret ptr getelementptr (i8, ptr @g, i64 -2)
1689+
; CHECK-NEXT: ret ptr getelementptr (i8, ptr @g, i64 254)
16911690
;
16921691
ret ptr getelementptr (i8, ptr getelementptr (i8, ptr @g, i8 127), i8 127)
16931692
}

‎llvm/test/Transforms/InstCombine/ptrtoint-nullgep.ll

+8-8
Original file line numberDiff line numberDiff line change
@@ -68,10 +68,10 @@ define void @constant_fold_ptrtoint_of_gep_of_nullgep() {
6868
; LLPARSER-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr inbounds (i8, ptr addrspace(1) null, i64 1234) to i64))
6969
; LLPARSER-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr (i8, ptr addrspace(1) null, i64 1234) to i64))
7070
; LLPARSER-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr (i8, ptr addrspace(1) null, i64 1234) to i64))
71-
; LLPARSER-NEXT: call void @use_i64(i64 0)
72-
; LLPARSER-NEXT: call void @use_i64(i64 0)
73-
; LLPARSER-NEXT: call void @use_i64(i64 0)
74-
; LLPARSER-NEXT: call void @use_i64(i64 0)
71+
; 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))
72+
; 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))
73+
; 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))
74+
; 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))
7575
; LLPARSER-NEXT: ret void
7676
;
7777
; INSTSIMPLIFY-LABEL: define {{[^@]+}}@constant_fold_ptrtoint_of_gep_of_nullgep() {
@@ -83,10 +83,10 @@ define void @constant_fold_ptrtoint_of_gep_of_nullgep() {
8383
; INSTSIMPLIFY-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr inbounds (i8, ptr addrspace(1) null, i64 1234) to i64))
8484
; INSTSIMPLIFY-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr (i8, ptr addrspace(1) null, i64 1234) to i64))
8585
; INSTSIMPLIFY-NEXT: call void @use_i64(i64 ptrtoint (ptr addrspace(1) getelementptr (i8, ptr addrspace(1) null, i64 1234) to i64))
86-
; INSTSIMPLIFY-NEXT: call void @use_i64(i64 0)
87-
; INSTSIMPLIFY-NEXT: call void @use_i64(i64 0)
88-
; INSTSIMPLIFY-NEXT: call void @use_i64(i64 0)
89-
; INSTSIMPLIFY-NEXT: call void @use_i64(i64 0)
86+
; 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))
87+
; 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))
88+
; 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))
89+
; 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))
9090
; INSTSIMPLIFY-NEXT: ret void
9191
;
9292
; INSTCOMBINE-LABEL: define {{[^@]+}}@constant_fold_ptrtoint_of_gep_of_nullgep() {

0 commit comments

Comments
 (0)