Keenuts added inline comments.
================ Comment at: llvm/unittests/TargetParser/TripleTest.cpp:1307 + EXPECT_TRUE(T.isSPIRV()); + T.setArch(Triple::spirv32); ---------------- Keenuts wrote: > 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). Opened https://discourse.llvm.org/t/rfc-spir-v-allow-architectures-with-no-set-pointer-size/72970 in case somebody has a better solution than this. I'd be in favor of returning 32 or 64 bits, and handle it in the backend, but might not be the smartest thing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155978/new/ https://reviews.llvm.org/D155978 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
