[SPIR-V] Add InferAddrSpaces pass to the backend #137766
Conversation
|
@llvm/pr-subscribers-backend-spir-v Author: Nathan Gauër (Keenuts) ChangesThis commit enables a pass in the backend which propagates the addrspace of the pointers down to the last use, making sure the addrspace remains consistent, and thus stripping any addrspacecast. This is required to lower LLVM-IR to logical SPIR-V, which does not support generic pointers. This is now required as HLSL emits several address spaces, and thus addrspacecasts in some cases: Example 1: resource access %handle = tail call target("spirv.VulkanBuffer", ...)
%rptr = @<!-- -->llvm.spv.resource.getpointer(%handle, ...);
%cptr = addrspacecast ptr addrspace(11) %rptr to ptr
%fptr = load i32, ptr %cptrExample 2: object methods define void @<!-- -->objectMethod(ptr %this) {
}
define void @<!-- -->foo(ptr addrspace(11) %object) {
call void @<!-- -->objectMethod(ptr addrspacecast(addrspace(11) %object to ptr));
}Full diff: https://github.com/llvm/llvm-project/pull/137766.diff 4 Files Affected:
diff --git a/llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp b/llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp
index 68286737b972f..1c253a9531a76 100644
--- a/llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp
@@ -32,6 +32,7 @@
#include "llvm/Passes/PassBuilder.h"
#include "llvm/Target/TargetOptions.h"
#include "llvm/Transforms/Scalar/Reg2Mem.h"
+#include "llvm/Transforms/Scalar.h"
#include "llvm/Transforms/Utils.h"
#include <optional>
@@ -190,6 +191,14 @@ void SPIRVPassConfig::addIRPasses() {
TargetPassConfig::addIRPasses();
if (TM.getSubtargetImpl()->isVulkanEnv()) {
+ // The frontend has a tendency to quickly addrspacecast pointers to the
+ // default address space, and relies on addrspacecast instructions at the
+ // boundaries. Vulkan does not allow such things, and we must keep the
+ // pointer address space stable.
+ // This pass will determine real address space of a pointer, and patch
+ // instructions removing Addrspacecasts.
+ addPass(createInferAddressSpacesPass(/* AddressSpace= */ 0));
+
// 1. Simplify loop for subsequent transformations. After this steps, loops
// have the following properties:
// - loops have a single entry edge (pre-header to loop header).
diff --git a/llvm/test/CodeGen/SPIRV/pointers/pointer-addrspacecast.ll b/llvm/test/CodeGen/SPIRV/pointers/pointer-addrspacecast.ll
new file mode 100644
index 0000000000000..4d5549dfab8d9
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/pointers/pointer-addrspacecast.ll
@@ -0,0 +1,36 @@
+; RUN: llc -verify-machineinstrs -O3 -mtriple=spirv-unknown-vulkan1.3-compute %s -o - | FileCheck %s
+; RUN: %if spirv-tools %{ llc -O3 -mtriple=spirv-unknown-vulkan1.3-compute %s -o - -filetype=obj | spirv-val %}
+
+; CHECK-DAG: %[[#uint:]] = OpTypeInt 32 0
+; CHECK-DAG: %[[#uint_0:]] = OpConstant %[[#uint]] 0
+; CHECK-DAG: %[[#ptr_uint:]] = OpTypePointer Private %[[#uint]]
+; CHECK-DAG: %[[#var:]] = OpVariable %[[#ptr_uint]] Private %[[#uint_0]]
+
+; CHECK-DAG: OpName %[[#func_simple:]] "simple"
+; CHECK-DAG: OpName %[[#func_chain:]] "chain"
+
+@global = internal addrspace(10) global i32 zeroinitializer
+
+define void @simple() {
+; CHECK: %[[#func_simple]] = OpFunction
+entry:
+ %ptr = getelementptr i32, ptr addrspace(10) @global, i32 0
+ %casted = addrspacecast ptr addrspace(10) %ptr to ptr
+ %val = load i32, ptr %casted
+; CHECK: %{{.*}} = OpLoad %[[#uint]] %[[#var]] Aligned 4
+ ret void
+}
+
+define void @chain() {
+; CHECK: %[[#func_chain]] = OpFunction
+entry:
+ %a = getelementptr i32, ptr addrspace(10) @global, i32 0
+ %b = addrspacecast ptr addrspace(10) %a to ptr
+ %c = getelementptr i32, ptr %b, i32 0
+ %d = addrspacecast ptr %c to ptr addrspace(10)
+ %e = addrspacecast ptr addrspace(10) %d to ptr
+
+ %val = load i32, ptr %e
+; CHECK: %{{.*}} = OpLoad %[[#uint]] %[[#var]] Aligned 4
+ ret void
+}
diff --git a/llvm/test/CodeGen/SPIRV/pointers/resource-addrspacecast-2.ll b/llvm/test/CodeGen/SPIRV/pointers/resource-addrspacecast-2.ll
new file mode 100644
index 0000000000000..93208c16ed4a5
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/pointers/resource-addrspacecast-2.ll
@@ -0,0 +1,54 @@
+; RUN: llc -verify-machineinstrs -O3 -mtriple=spirv-unknown-vulkan1.3-compute %s -o - | FileCheck %s --match-full-lines
+; RUN: %if spirv-tools %{ llc -O3 -mtriple=spirv-unknown-vulkan1.3-compute %s -o - -filetype=obj | spirv-val %}
+
+; FIXME(134119): enable-this once Offset decoration are added.
+; XFAIL: spirv-tools
+
+%S2 = type { { [10 x { i32, i32 } ] }, i32 }
+
+; CHECK-DAG: %[[#uint:]] = OpTypeInt 32 0
+; CHECK-DAG: %[[#uint_0:]] = OpConstant %[[#uint]] 0
+; CHECK-DAG: %[[#uint_1:]] = OpConstant %[[#uint]] 1
+; CHECK-DAG: %[[#uint_3:]] = OpConstant %[[#uint]] 3
+; CHECK-DAG: %[[#uint_10:]] = OpConstant %[[#uint]] 10
+; CHECK-DAG: %[[#uint_11:]] = OpConstant %[[#uint]] 11
+; CHECK-DAG: %[[#ptr_StorageBuffer_uint:]] = OpTypePointer StorageBuffer %[[#uint]]
+
+; CHECK-DAG: %[[#t_s2_s_a_s:]] = OpTypeStruct %[[#uint]] %[[#uint]]
+; CHECK-DAG: %[[#t_s2_s_a:]] = OpTypeArray %[[#t_s2_s_a_s]] %[[#uint_10]]
+; CHECK-DAG: %[[#t_s2_s:]] = OpTypeStruct %[[#t_s2_s_a]]
+; CHECK-DAG: %[[#t_s2:]] = OpTypeStruct %[[#t_s2_s]] %[[#uint]]
+
+; CHECK-DAG: %[[#ptr_StorageBuffer_struct:]] = OpTypePointer StorageBuffer %[[#t_s2]]
+; CHECK-DAG: %[[#rarr:]] = OpTypeRuntimeArray %[[#t_s2]]
+; CHECK-DAG: %[[#rarr_struct:]] = OpTypeStruct %[[#rarr]]
+; CHECK-DAG: %[[#spirv_VulkanBuffer:]] = OpTypePointer StorageBuffer %[[#rarr_struct]]
+
+declare target("spirv.VulkanBuffer", [0 x %S2], 12, 1) @llvm.spv.resource.handlefrombinding.tspirv.VulkanBuffer_a0s_Ss_12_1t(i32, i32, i32, i32, i1)
+
+define void @main() "hlsl.numthreads"="1,1,1" "hlsl.shader"="compute" {
+entry:
+ %handle = tail call target("spirv.VulkanBuffer", [0 x %S2], 12, 1) @llvm.spv.resource.handlefrombinding.tspirv.VulkanBuffer_a0s_Ss_12_1t(i32 0, i32 0, i32 1, i32 0, i1 false)
+; CHECK: %[[#resource:]] = OpVariable %[[#spirv_VulkanBuffer]] StorageBuffer
+
+ %ptr = tail call noundef align 4 dereferenceable(4) ptr addrspace(11) @llvm.spv.resource.getpointer.p11.tspirv.VulkanBuffer_a0s_Ss_12_1t(target("spirv.VulkanBuffer", [0 x %S2], 12, 1) %handle, i32 0)
+; CHECK: %[[#a:]] = OpCopyObject %[[#spirv_VulkanBuffer]] %[[#resource]]
+; CHECK: %[[#b:]] = OpAccessChain %[[#ptr_StorageBuffer_struct]] %[[#a:]] %[[#uint_0]] %[[#uint_0]]
+ %casted = addrspacecast ptr addrspace(11) %ptr to ptr
+
+; CHECK: %[[#ptr2:]] = OpInBoundsAccessChain %[[#ptr_StorageBuffer_uint]] %[[#b:]] %[[#uint_0]] %[[#uint_0]] %[[#uint_3]] %[[#uint_1]]
+ %ptr2 = getelementptr inbounds %S2, ptr %casted, i64 0, i32 0, i32 0, i32 3, i32 1
+
+; CHECK: OpStore %[[#ptr2]] %[[#uint_10]] Aligned 4
+ store i32 10, ptr %ptr2, align 4
+
+; Another store, but this time using LLVM's ability to load the first element
+; without an explicit GEP. The backend has to determine the ptr type and
+; generate the appropriate access chain.
+; CHECK: %[[#ptr3:]] = OpInBoundsAccessChain %[[#ptr_StorageBuffer_uint]] %[[#b:]] %[[#uint_0]] %[[#uint_0]] %[[#uint_0]] %[[#uint_0]]
+; CHECK: OpStore %[[#ptr3]] %[[#uint_11]] Aligned 4
+ store i32 11, ptr %casted, align 4
+ ret void
+}
+
+declare ptr addrspace(11) @llvm.spv.resource.getpointer.p11.tspirv.VulkanBuffer_a0s_S2s_12_1t(target("spirv.VulkanBuffer", [0 x %S2], 12, 1), i32)
diff --git a/llvm/test/CodeGen/SPIRV/pointers/resource-addrspacecast.ll b/llvm/test/CodeGen/SPIRV/pointers/resource-addrspacecast.ll
new file mode 100644
index 0000000000000..24a50c7177340
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/pointers/resource-addrspacecast.ll
@@ -0,0 +1,37 @@
+; RUN: llc -verify-machineinstrs -O3 -mtriple=spirv-unknown-vulkan1.3-compute %s -o - | FileCheck %s
+; RUN: %if spirv-tools %{ llc -O3 -mtriple=spirv-unknown-vulkan1.3-compute %s -o - -filetype=obj | spirv-val %}
+
+; FIXME(134119): enable-this once Offset decoration are added.
+; XFAIL: spirv-tools
+
+%struct.S = type { i32 }
+
+; CHECK-DAG: %[[#uint:]] = OpTypeInt 32 0
+; CHECK-DAG: %[[#uint_0:]] = OpConstant %[[#uint]] 0
+; CHECK-DAG: %[[#uint_10:]] = OpConstant %[[#uint]] 10
+; CHECK-DAG: %[[#ptr_StorageBuffer_uint:]] = OpTypePointer StorageBuffer %[[#uint]]
+; CHECK-DAG: %[[#struct:]] = OpTypeStruct %[[#uint]]
+; CHECK-DAG: %[[#ptr_StorageBuffer_struct:]] = OpTypePointer StorageBuffer %[[#struct]]
+; CHECK-DAG: %[[#rarr:]] = OpTypeRuntimeArray %[[#struct]]
+; CHECK-DAG: %[[#rarr_struct:]] = OpTypeStruct %[[#rarr]]
+; CHECK-DAG: %[[#spirv_VulkanBuffer:]] = OpTypePointer StorageBuffer %[[#rarr_struct]]
+
+declare target("spirv.VulkanBuffer", [0 x %struct.S], 12, 1) @llvm.spv.resource.handlefrombinding.tspirv.VulkanBuffer_a0s_struct.Ss_12_1t(i32, i32, i32, i32, i1)
+
+define void @main() "hlsl.numthreads"="1,1,1" "hlsl.shader"="compute" {
+entry:
+ %handle = tail call target("spirv.VulkanBuffer", [0 x %struct.S], 12, 1) @llvm.spv.resource.handlefrombinding.tspirv.VulkanBuffer_a0s_struct.Ss_12_1t(i32 0, i32 0, i32 1, i32 0, i1 false)
+; CHECK: %[[#resource:]] = OpVariable %[[#spirv_VulkanBuffer]] StorageBuffer
+
+ %ptr = tail call noundef align 4 dereferenceable(4) ptr addrspace(11) @llvm.spv.resource.getpointer.p11.tspirv.VulkanBuffer_a0s_struct.Ss_12_1t(target("spirv.VulkanBuffer", [0 x %struct.S], 12, 1) %handle, i32 0)
+; CHECK: %[[#a:]] = OpCopyObject %[[#spirv_VulkanBuffer]] %[[#resource]]
+; CHECK: %[[#b:]] = OpAccessChain %[[#ptr_StorageBuffer_struct]] %[[#a:]] %[[#uint_0]] %[[#uint_0]]
+; CHECK: %[[#c:]] = OpInBoundsAccessChain %[[#ptr_StorageBuffer_uint]] %[[#b:]] %[[#uint_0]]
+ %casted = addrspacecast ptr addrspace(11) %ptr to ptr
+
+; CHECK: OpStore %[[#c]] %[[#uint_10]] Aligned 4
+ store i32 10, ptr %casted, align 4
+ ret void
+}
+
+declare ptr addrspace(11) @llvm.spv.resource.getpointer.p11.tspirv.VulkanBuffer_a0s_struct.Ss_12_1t(target("spirv.VulkanBuffer", [0 x %struct.S], 12, 1), i32)
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
This commit enables a pass in the backend which propagates the
addrspace of the pointers down to the last use, making sure the
addrspace remains consistent, and thus stripping any addrspacecast.
This is required to lower LLVM-IR to logical SPIR-V, which does not
support generic pointers.
This is now required as HLSL emits several address spaces, and thus
addrspacecasts in some cases:
Example 1: resource access
```llvm
%handle = tail call target("spirv.VulkanBuffer", ...)
%rptr = @llvm.spv.resource.getpointer(%handle, ...);
%cptr = addrspacecast ptr addrspace(11) %rptr to ptr
%fptr = load i32, ptr %cptr
```
Example 2: object methods
```llvm
define void @objectmethod(ptr %this) {
}
define void @foo(ptr addrspace(11) %object) {
call void @objectmethod(ptr addrspacecast(addrspace(11) %object to ptr));
}
```
Keenuts
force-pushed
the
add-infer-addrspace
branch
from
18ef956 to
1225c2f
Compare
| // pointer address space stable. | ||
| // This pass will determine real address space of a pointer, and patch | ||
| // instructions removing Addrspacecasts. | ||
| addPass(createInferAddressSpacesPass(/* AddressSpace= */ 0)); |
There was a problem hiding this comment.
The pass can call TargetTransformInfo::getFlatAddressSpace() to get the address space used as flat, so it would make sense to implement SPIRVTTIImpl::getFlatAddressSpace() for this and make it compatible with compute (which would use 4 in this case). There is other passes in LLVM that could use this information as well.
I'm saying this while not knowing much about hlsl.
There was a problem hiding this comment.
I'm not sure about that. In the logical SPIRV world (outside of buffer content), there are no relationships between addresses like a flat address space would give, pointers are abstract and have no numerical value.
From the HLSL point of view, it's a bit the same: those address spaces are "spaces", but it's not defined if it's flat or not (we don't do pointer arithmetic for ex).
(once again, this is valid for let's say 2 input variables, not for data inside a resource like a buffer)
So saying I feel saying "0 is flat" wouldn't be right in this case.
There was a problem hiding this comment.
In the logical SPIRV world (outside of buffer content), there are no relationships between addresses like a flat address space would give, pointers are abstract and have no numerical value.
I know SPIR-V ;)
My point is not about SPIR-V, but about the LLVM representation you have for this target. I haven't checked, but I suspect you get these addrspacecast because LangAS::Default maps to 0. So, 0 is defacto used as your (unwanted) flat address space and you use the infer pass to workaround it. I agree 0 is flat isn't right from a SPIR-V perspective, although I think it is for the LLVM representation you use.
I'm suggesting this because it makes sense for the backend in general to have this hook implemented (and the infer pass enabled in general as well, but I don't think it should be enabled with this patch).
There was a problem hiding this comment.
Thanks for the info. I'm quite fuzzy on this, so let me know If I'm missing something:
In our case yes, we map to 0 -> SPIRV::Function, but it is true other targets map this to other values (Sycl says DefaultAS is 4 for ex, which is converted to SPIRV::Generic).
But there is something I don't understand: It feels like this pass is using getFlatAddressSpace() to get the AS Clang would generally addrspacecast eagerly to, and possibly keep other valid ones. But I don't get why the AS being 'flat' is important for this specific pass.
Hence why I'm not eager to add the getFlatAddressSpace() in the target, because I feel the name implies something more than just this is the default AS.
Putting this aside, the LangAS -> int mapping is done in the FE, so I don't think we should be doing a isSycl() kind of check to pick what the default AS translates to.
In that regards, it is indeed weird to put addressSpace = 0 as if that was always the case, but wouldn't any other condition we put as-arbitrary?
There was a problem hiding this comment.
Sycl says DefaultAS is 4 for ex, which is converted to SPIRV::Generic
It is a bit more subtle than this but in short yes. Regardless, better to leave languages the discussion, how it is mapped to the AST and lower to the backend convention is the FE's problem. What matters is the backend convention (which sounds confusing).
so I don't think we should be doing a isSycl() kind of check to pick what the default AS translates to.
I haven't suggested anything like this, this is clang's problem. I'm only suggesting to select the flat address space according to target SPIR-V environment (because they seem different).
In our case yes, we map to 0 -> SPIRV::Function
oh so 0 is still mapped to Function/Private, sorry I assumed distinct address space. I understand a bit better your position. That could be an issue.
But there is something I don't understand: It feels like this pass is using getFlatAddressSpace() to get the AS Clang would generally addrspacecast eagerly to, and possibly keep other valid ones. But I don't get why the AS being 'flat' is important for this specific pass.
The pass is there to remove use of the "generic" address space (understand generic as the opencl one), LLVM uses "flat" as a more neutral term for this, but basically still caries the assumptions you have in cuda, opencl etc (the pass was historically added to llvm with the NVPTX backend and later on moved so the AMDGPU could use it).
The fact that 0 is used for the Function SC isn't an issue as such, NVPTX also uses 0 for both the flat AS and its stack.
What the default AS means for the backend ?
it is indeed weird to put addressSpace = 0 as if that was always the case, but wouldn't any other condition we put as-arbitrary?
IMHO, it would be more clean to use 0 exclusively for Function/Private and something else for this "default". But that's would be a large change also impacting clang (and MLIR ?).
That being said, NVPTX uses 0 for both its private and flat address space.
I understand your reluctance a bit better, I still think you are defacto using 0 as a flat address space, but I start to feel like there is a bit of nitpicking at this stage. So feel free to consider this resolved.
This commit enables a pass in the backend which propagates the addrspace of the pointers down to the last use, making sure the addrspace remains consistent, and thus stripping any addrspacecast. This is required to lower LLVM-IR to logical SPIR-V, which does not support generic pointers.
This is now required as HLSL emits several address spaces, and thus addrspacecasts in some cases:
Example 1: resource access
Example 2: object methods