Skip to content

InstCombine: Fix a crash in PointerReplacer when constructing a new PHI #130256

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 10, 2025
Merged

InstCombine: Fix a crash in PointerReplacer when constructing a new PHI #130256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2831949
InstCombine: Fix a crash in `PointerReplacer` when constructing a PHI
changpeng Mar 7, 2025
7ef6ac0
InstCombine: Fix a crash in PointerReplacer when constructing a new PHI
changpeng Mar 7, 2025
5d67890
InstCombine: Fix a crash in PointerReplacer when constructing a new PHI
changpeng Mar 7, 2025
1c5e5ba
InstCombine: Fix a crash in PointerReplacer when constructing a new PHI
changpeng Mar 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,10 @@ void PointerReplacer::replace(Instruction *I) {

IC.InsertNewInstWith(NewI, LT->getIterator());
IC.replaceInstUsesWith(*LT, NewI);
WorkMap[LT] = NewI;
// LT has actually been replaced by NewI. It is useless to insert LT into
// the map. Instead, we insert NewI into the map to indicate this is the
// replacement (new value).
WorkMap[NewI] = NewI;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just not add it to the map? I'm not sure what the point of the identity mapping would be

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just not add it to the map? I'm not sure what the point of the identity mapping would be

The whole logic is that, when replacing a PHI instruction, all its operands are expected to have already been replaced and can be found in the map. So getReplacement (Operand) should always return the replacement (never a NULL).
Type *NewTy = getReplacement(PHI->getIncomingValue(0))->getType();

However, when a load is replaced, all the uses of the load has already been actually replaced by the new load:
IC.replaceInstUsesWith(*LT, NewI);

We will never search the map with the original load (LT). And we expect to find the replacement for the new load.
So we have to add the newI into the map instead.

} else if (auto *PHI = dyn_cast<PHINode>(I)) {
Type *NewTy = getReplacement(PHI->getIncomingValue(0))->getType();
auto *NewPHI = PHINode::Create(NewTy, PHI->getNumIncomingValues(),
Expand Down
42 changes: 42 additions & 0 deletions llvm/test/Transforms/InstCombine/AMDGPU/phi-with-incoming-from-load.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
; RUN: opt -passes=instcombine -S -o - %s | FileCheck %s

target triple = "amdgcn-amd-amdhsa"

%double_double = type { double, double }

declare void @llvm.memcpy.p0.p0.i64(ptr, ptr, i64, i1)

define void @_test(ptr addrspace(4) byref(%double_double) align 8 %in) {
; CHECK-LABEL: define void @_test(
; CHECK-SAME: ptr addrspace(4) byref([[DOUBLE_DOUBLE:%.*]]) align 8 [[IN:%.*]]) {
; CHECK-NEXT: [[ENTRY:.*]]:
; CHECK-NEXT: [[ALPHA_UNION:%.*]] = addrspacecast ptr addrspace(4) [[IN]] to ptr
; CHECK-NEXT: [[LOAD:%.*]] = load i8, ptr addrspace(5) null, align 1
; CHECK-NEXT: [[LOADEDV:%.*]] = trunc i8 [[LOAD]] to i1
; CHECK-NEXT: br i1 [[LOADEDV]], label %[[COND_END:.*]], label %[[COND_FALSE:.*]]
; CHECK: [[COND_FALSE]]:
; CHECK-NEXT: [[TMP0:%.*]] = load ptr, ptr addrspace(4) [[IN]], align 8
; CHECK-NEXT: br label %[[COND_END]]
; CHECK: [[COND_END]]:
; CHECK-NEXT: [[COND1:%.*]] = phi ptr [ [[TMP0]], %[[COND_FALSE]] ], [ [[ALPHA_UNION]], %[[ENTRY]] ]
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 1 dereferenceable(16) poison, ptr noundef nonnull align 1 dereferenceable(16) [[COND1]], i64 16, i1 false)
; CHECK-NEXT: ret void
;
entry:
%coerce = alloca %double_double, align 8, addrspace(5)
%alpha_union = addrspacecast ptr addrspace(5) %coerce to ptr
call void @llvm.memcpy.p5.p4.i64(ptr addrspace(5) align 8 %coerce, ptr addrspace(4) align 8 %in, i64 16, i1 false)
%load1 = load i8, ptr addrspace(5) null, align 1
%loadedv = trunc i8 %load1 to i1
br i1 %loadedv, label %cond.end, label %cond.false

cond.false:
%load2 = load ptr, ptr addrspace(5) %coerce, align 8
br label %cond.end

cond.end:
%cond = phi ptr [ %load2, %cond.false ], [ %alpha_union, %entry ]
call void @llvm.memcpy.p0.p0.i64(ptr poison, ptr %cond, i64 16, i1 false)
ret void
}
Loading