[llvm] [InferAddrSpaces] Correctly replace identical operands of insts (PR #82610)
Pierre van Houtryve via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 22 04:10:42 PST 2024
https://github.com/Pierre-vh updated https://github.com/llvm/llvm-project/pull/82610
>From 4b7b6967c98cf820079bfa525bac0c78e04165ff Mon Sep 17 00:00:00 2001
From: pvanhout <pierre.vanhoutryve at amd.com>
Date: Thu, 22 Feb 2024 12:19:18 +0100
Subject: [PATCH 1/3] [InferAddrSpaces] Correctly replace multiple occurences
of a value in an instruction
It's important for PHI nodes because if a PHI node
has multiple edges coming from the same block, we can have the same incoming value multiple times in the list of incoming values.
All of those need to be consistent (exact same Value*) otherwise verifier complains.
---
.../Transforms/Scalar/InferAddressSpaces.cpp | 4 +-
.../AMDGPU/multiple-uses-of-val.ll | 69 +++++++++++++++++++
2 files changed, 72 insertions(+), 1 deletion(-)
create mode 100644 llvm/test/Transforms/InferAddressSpaces/AMDGPU/multiple-uses-of-val.ll
diff --git a/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp b/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
index 1bf50d79e5331e..038171d3a2bdb3 100644
--- a/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
+++ b/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
@@ -1311,7 +1311,9 @@ bool InferAddressSpacesImpl::rewriteWithNewAddressSpaces(
while (isa<PHINode>(InsertPos))
++InsertPos;
- U.set(new AddrSpaceCastInst(NewV, V->getType(), "", &*InsertPos));
+ // This instruction may contain multiple uses of V, update them all.
+ U.getUser()->replaceUsesOfWith(
+ V, new AddrSpaceCastInst(NewV, V->getType(), "", &*InsertPos));
} else {
U.set(ConstantExpr::getAddrSpaceCast(cast<Constant>(NewV),
V->getType()));
diff --git a/llvm/test/Transforms/InferAddressSpaces/AMDGPU/multiple-uses-of-val.ll b/llvm/test/Transforms/InferAddressSpaces/AMDGPU/multiple-uses-of-val.ll
new file mode 100644
index 00000000000000..717bd09897732b
--- /dev/null
+++ b/llvm/test/Transforms/InferAddressSpaces/AMDGPU/multiple-uses-of-val.ll
@@ -0,0 +1,69 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -mtriple=amdgcn-amd-amdhsa -mcpu=gfx90a -S -passes=infer-address-spaces --verify-each %s | FileCheck %s
+
+; Inst can use a value multiple time. When we're inserting an addrspacecast to flat,
+; it's important all the identical uses use an indentical replacement, especially
+; for PHIs.
+
+define amdgpu_kernel void @test_phi() {
+; CHECK-LABEL: @test_phi(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[LOADED_PTR:%.*]] = load ptr, ptr addrspace(4) null, align 8
+; CHECK-NEXT: [[TMP0:%.*]] = addrspacecast ptr [[LOADED_PTR]] to ptr addrspace(1)
+; CHECK-NEXT: br label [[BB0:%.*]]
+; CHECK: bb0:
+; CHECK-NEXT: [[GEP:%.*]] = getelementptr i64, ptr addrspace(1) [[TMP0]], i64 3
+; CHECK-NEXT: [[TMP1:%.*]] = addrspacecast ptr addrspace(1) [[GEP]] to ptr
+; CHECK-NEXT: switch i32 0, label [[END:%.*]] [
+; CHECK-NEXT: i32 1, label [[END]]
+; CHECK-NEXT: i32 4, label [[END]]
+; CHECK-NEXT: i32 5, label [[BB1:%.*]]
+; CHECK-NEXT: ]
+; CHECK: bb1:
+; CHECK-NEXT: [[TMP2:%.*]] = load double, ptr addrspace(1) [[GEP]], align 16
+; CHECK-NEXT: br label [[END]]
+; CHECK: end:
+; CHECK-NEXT: [[RETVAL_SROA_0_0_I569_PH:%.*]] = phi ptr [ null, [[BB1]] ], [ [[TMP1]], [[BB0]] ], [ [[TMP1]], [[BB0]] ], [ [[TMP1]], [[BB0]] ]
+; CHECK-NEXT: ret void
+;
+entry:
+ %loaded.ptr = load ptr, ptr addrspace(4) null, align 8
+ br label %bb0
+
+bb0:
+ %gep = getelementptr i64, ptr %loaded.ptr, i64 3
+ switch i32 0, label %end [
+ i32 1, label %end
+ i32 4, label %end
+ i32 5, label %bb1
+ ]
+
+bb1:
+ %0 = load double, ptr %gep, align 16
+ br label %end
+
+end:
+ %retval.sroa.0.0.i569.ph = phi ptr [ null, %bb1 ], [ %gep, %bb0 ], [ %gep, %bb0 ], [ %gep, %bb0 ]
+ ret void
+}
+
+declare void @uses_ptrs(ptr, ptr, ptr)
+
+; We shouldn't treat PHIs differently, even other users should have the same treatment.
+; All occurences of %gep are replaced with an identical value.
+define amdgpu_kernel void @test_other() {
+; CHECK-LABEL: @test_other(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[LOADED_PTR:%.*]] = load ptr, ptr addrspace(4) null, align 8
+; CHECK-NEXT: [[TMP0:%.*]] = addrspacecast ptr [[LOADED_PTR]] to ptr addrspace(1)
+; CHECK-NEXT: [[TMP1:%.*]] = addrspacecast ptr addrspace(1) [[TMP0]] to ptr
+; CHECK-NEXT: [[GEP:%.*]] = getelementptr i64, ptr [[TMP1]], i64 3
+; CHECK-NEXT: call void @uses_ptrs(ptr [[GEP]], ptr [[GEP]], ptr [[GEP]])
+; CHECK-NEXT: ret void
+;
+entry:
+ %loaded.ptr = load ptr, ptr addrspace(4) null, align 8
+ %gep = getelementptr i64, ptr %loaded.ptr, i64 3
+ call void @uses_ptrs(ptr %gep, ptr %gep, ptr %gep)
+ ret void
+}
>From f7734c7f50b365299ebf07b866eb97ad9ee8ee5f Mon Sep 17 00:00:00 2001
From: pvanhout <pierre.vanhoutryve at amd.com>
Date: Thu, 22 Feb 2024 13:07:14 +0100
Subject: [PATCH 2/3] fix all instances of u.set
---
llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp b/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
index 038171d3a2bdb3..63e8c1bfaf4901 100644
--- a/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
+++ b/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
@@ -1221,6 +1221,7 @@ bool InferAddressSpacesImpl::rewriteWithNewAddressSpaces(
Value::use_iterator I, E, Next;
for (I = V->use_begin(), E = V->use_end(); I != E;) {
Use &U = *I;
+ User *CurUser = U.getUser();
// Some users may see the same pointer operand in multiple operands. Skip
// to the next instruction.
@@ -1231,11 +1232,10 @@ bool InferAddressSpacesImpl::rewriteWithNewAddressSpaces(
// If V is used as the pointer operand of a compatible memory operation,
// sets the pointer operand to NewV. This replacement does not change
// the element type, so the resultant load/store is still valid.
- U.set(NewV);
+ CurUser->replaceUsesOfWith(V, NewV);
continue;
}
- User *CurUser = U.getUser();
// Skip if the current user is the new value itself.
if (CurUser == NewV)
continue;
@@ -1312,10 +1312,10 @@ bool InferAddressSpacesImpl::rewriteWithNewAddressSpaces(
while (isa<PHINode>(InsertPos))
++InsertPos;
// This instruction may contain multiple uses of V, update them all.
- U.getUser()->replaceUsesOfWith(
+ CurUser->replaceUsesOfWith(
V, new AddrSpaceCastInst(NewV, V->getType(), "", &*InsertPos));
} else {
- U.set(ConstantExpr::getAddrSpaceCast(cast<Constant>(NewV),
+ CurUser->replaceUsesOfWith(V, ConstantExpr::getAddrSpaceCast(cast<Constant>(NewV),
V->getType()));
}
}
>From a11c08867ab3e210851d102cc567dea4fd2b4ae9 Mon Sep 17 00:00:00 2001
From: pvanhout <pierre.vanhoutryve at amd.com>
Date: Thu, 22 Feb 2024 13:10:31 +0100
Subject: [PATCH 3/3] clang-format
---
llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp b/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
index 63e8c1bfaf4901..851eab04c8dbb2 100644
--- a/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
+++ b/llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
@@ -1315,8 +1315,9 @@ bool InferAddressSpacesImpl::rewriteWithNewAddressSpaces(
CurUser->replaceUsesOfWith(
V, new AddrSpaceCastInst(NewV, V->getType(), "", &*InsertPos));
} else {
- CurUser->replaceUsesOfWith(V, ConstantExpr::getAddrSpaceCast(cast<Constant>(NewV),
- V->getType()));
+ CurUser->replaceUsesOfWith(
+ V, ConstantExpr::getAddrSpaceCast(cast<Constant>(NewV),
+ V->getType()));
}
}
}
More information about the llvm-commits
mailing list