Skip to content

Commit 9594c7f

Browse files
committed
Fix incorrect codegen with respect to GEPs #85333
As mentioned in #68882 and https://discourse.llvm.org/t/rfc-replacing-getelementptr-with-ptradd/68699 Gep arithmetic isn't consistent with different types. GVNSink didn't realize this and sank all geps as long as their operands can be wired via PHIs in a post-dominator.
1 parent a11a432 commit 9594c7f

File tree

2 files changed

+66
-8
lines changed

2 files changed

+66
-8
lines changed

llvm/lib/Transforms/Scalar/GVNSink.cpp

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -719,21 +719,32 @@ GVNSink::analyzeInstructionForSinking(LockstepReverseIterator &LRI,
719719
// try and continue making progress.
720720
Instruction *I0 = NewInsts[0];
721721

722-
// If all instructions that are going to participate don't have the same
723-
// number of operands, we can't do any useful PHI analysis for all operands.
724-
auto hasDifferentNumOperands = [&I0](Instruction *I) {
725-
return I->getNumOperands() != I0->getNumOperands();
722+
auto hasDifferentOperands = [&I0](Instruction *I) {
723+
// If all instructions that are going to participate don't have the same
724+
// number of operands, we can't do any useful PHI analysis for all operands.
725+
if (I->getNumOperands() != I0->getNumOperands())
726+
return true;
727+
// Having different source element types may result in incorrect
728+
// pointer arithmetic on geps(github.com/llvm/llvm-project/issues/85333)
729+
if (auto *II = dyn_cast<GetElementPtrInst>(I)) {
730+
auto I0I = cast<GetElementPtrInst>(I0);
731+
return II->getSourceElementType() != I0I->getSourceElementType();
732+
}
733+
return false;
726734
};
727-
if (any_of(NewInsts, hasDifferentNumOperands))
735+
736+
if (any_of(NewInsts, hasDifferentOperands))
728737
return std::nullopt;
729738

730739
for (unsigned OpNum = 0, E = I0->getNumOperands(); OpNum != E; ++OpNum) {
731740
ModelledPHI PHI(NewInsts, OpNum, ActivePreds);
732741
if (PHI.areAllIncomingValuesSame())
733742
continue;
734-
if (!canReplaceOperandWithVariable(I0, OpNum))
735-
// We can 't create a PHI from this instruction!
736-
return std::nullopt;
743+
for (auto &Candidate : NewInsts) {
744+
if (!canReplaceOperandWithVariable(Candidate, OpNum))
745+
// We can 't create a PHI from this instruction!
746+
return std::nullopt;
747+
}
737748
if (NeededPHIs.count(PHI))
738749
continue;
739750
if (!PHI.areAllIncomingValuesSameType())
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
; RUN: opt -passes=gvn-sink -S %s | FileCheck %s
2+
3+
target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
4+
target triple = "thumbv7em-none-unknown-eabi"
5+
6+
%"struct.std::pair" = type <{ i32, %struct.substruct, [2 x i8] }>
7+
%struct.substruct = type { i8, i8 }
8+
%"struct.std::random_access_iterator_tag" = type { i8 }
9+
10+
; CHECK: if.end6
11+
; CHECK: %incdec.ptr.sink = phi ptr [ %incdec.ptr, %if.then ], [ %incdec.ptr4, %if.then3 ], [ %add.ptr, %if.else5 ]
12+
; CHECK-NEXT: store ptr %incdec.ptr.sink, ptr %__i, align 4
13+
14+
define linkonce_odr dso_local void @__advance(ptr noundef nonnull align 4 dereferenceable(4) %__i, i32 noundef %__n) local_unnamed_addr {
15+
entry:
16+
%0 = call i1 @llvm.is.constant.i32(i32 %__n)
17+
%cmp = icmp eq i32 %__n, 1
18+
%or.cond = and i1 %0, %cmp
19+
br i1 %or.cond, label %if.then, label %if.else
20+
21+
if.then: ; preds = %entry
22+
%1 = load ptr, ptr %__i, align 4
23+
%incdec.ptr = getelementptr inbounds i8, ptr %1, i32 8
24+
store ptr %incdec.ptr, ptr %__i, align 4
25+
br label %if.end6
26+
27+
if.else: ; preds = %entry
28+
%2 = call i1 @llvm.is.constant.i32(i32 %__n)
29+
%cmp2 = icmp eq i32 %__n, -1
30+
%or.cond7 = and i1 %2, %cmp2
31+
br i1 %or.cond7, label %if.then3, label %if.else5
32+
33+
if.then3: ; preds = %if.else
34+
%3 = load ptr, ptr %__i, align 4
35+
%incdec.ptr4 = getelementptr inbounds i8, ptr %3, i32 -8
36+
store ptr %incdec.ptr4, ptr %__i, align 4
37+
br label %if.end6
38+
39+
if.else5: ; preds = %if.else
40+
%4 = load ptr, ptr %__i, align 4
41+
%add.ptr = getelementptr inbounds %"struct.std::pair", ptr %4, i32 %__n
42+
store ptr %add.ptr, ptr %__i, align 4
43+
br label %if.end6
44+
45+
if.end6: ; preds = %if.then3, %if.else5, %if.then
46+
ret void
47+
}

0 commit comments

Comments
 (0)