[llvm] [SPIR-V] Add InferAddrSpaces pass to the backend (PR #137766)

[Victor Lomuller via llvm-commits]

Nathan =?utf-8?q?Gauër?= <brioche at google.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/137766 at github.com>


================
@@ -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));
----------------
Naghasan wrote:

> 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.

https://github.com/llvm/llvm-project/pull/137766