[PATCH] D155978: [SPIRV] Add SPIR-V logical triple.

[Nathan Gauër via Phabricator via cfe-commits]

Keenuts added inline comments.


================
Comment at: llvm/unittests/TargetParser/TripleTest.cpp:1307
+  EXPECT_TRUE(T.isSPIRV());
+
   T.setArch(Triple::spirv32);
----------------
Anastasia wrote:
> pmatos wrote:
> > Keenuts wrote:
> > > pmatos wrote:
> > > > I am not well-versed in SPIRV for gfx but if we are using 32bits in SPIRV logical, can't we reuse spirv32? Is there some sort of strong reason not to or adding spirv for logical spirv as an alternative to spirv32 and spirv64 is just easier?
> > > This is a very valid question! And I'm not sure of the best option.
> > > My understanding is: in logical SPIR-V, there are no notion of pointer size, or architecture size. We cannot offset pointers by a sizeof(), or do such things.
> > > 
> > > But the options I had didn't seem to allow this kind of "undefined architecture".
> > > I chose 32bits here because the IDs are 32bit. But pointer addresses are not, so returning 32bit is not quite right either.
> > > 
> > > 
> > This is a tricky one tbh - I would have to do a bit more research to have a more insighful reply here. Maybe @mpaszkowski or @Anastasia can add more.
> I think to do this properly in LLVM would require an IR extension or something, it is maybe worth starting RFC to discuss this.
> 
> Out of curiosity do you have any code that can be compiled from any GFX source language that would need a pointer size to be emitted in IR? If there is no code that can be written in such a way then using one fixed pointer width could be ok. Then the SPIR-V backend should just recognise it and lower as required. Otherwise target configurable types might be useful: https://llvm.org/docs/LangRef.html#target-extension-type
> 
> In general we tried to avoid adding bitwidth neutral SPIR-V triple originally just because LLVM always requires a pointer width. We were thinking of adding vulkan as a component to the existing SPIR-V 34|64 targets https://discourse.llvm.org/t/setting-version-environment-and-extensions-for-spir-v-target.
Hello!

Thanks for the feedback!
I asked around, and it seems that no, we cannot write code that pointer arithmetic or requires the pointer size that I know of.
The code that could require is changes the memory modal to something else, like this proposal: https://github.com/microsoft/hlsl-specs/blob/main/proposals/0010-vk-buffer-ref.md
But here, the memory model is PhysicalStorageBuffer64, which tells us pointers are 64bit.

We also have the SPV_KHR_variable_pointers extension (https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/KHR/SPV_KHR_variable_pointers.asciidoc)
which specifically says pointers have no size or bit pattern...


> Otherwise target configurable types might be useful

Not familiar enough with LLVM/Clang to fully understand, so reformulating we'd have:
- the target would return 32-bit, even if we have no pointer size
- the IR would not use classic pointers at all, since their type would be misleading.
- the IR would use this newly created type (let's call is LogicalPointer) instead of real pointers.
- the backend would then convert this new type to something else when lowering.

If that's the case, seems fair, I'd follow this advice.

> We were thinking of adding vulkan as a component to the existing SPIR-V 34|64 targets
Same idea here, add VulkanX to the OSType part of the triple (replacing the ShaderModel used for DXIL).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155978/new/

https://reviews.llvm.org/D155978