Clang implements SPIR-V with both Physical32 and Physical64 addressing
models. This commit adds a new triple value for the Logical
addressing model.
This will be required to add the graphics part of the SPIR-V backend (targeting vulkan for now).
Details
- Reviewers
Anastasia mpaszkowski - Commits
- rG53b6a169e453: [SPIR-V] Add SPIR-V logical triple.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
git-clang-format on changed files.
CI complains because the clang-format is doesn't match if I don't run this. But this changes a lot of unrelated lines (which seems to be frowned upon from the contribution guide). So unsure about this one.
Thanks for the patch. Left a few comments. Others can take a better look, I am sure. Maybe @iliya-diyachkov or @zuban32 can take a look as well.
llvm/include/llvm/TargetParser/Triple.h | ||
---|---|---|
163 | No need to reindent the whole block to add a single line. | |
llvm/lib/TargetParser/Triple.cpp | ||
74 | This should be in the same line. | |
447 | Same as above. I guess there's no need to reindent a whole block. | |
598 | Whole block reindent. | |
llvm/unittests/TargetParser/TripleTest.cpp | ||
1289 | 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? |
Thanks for the review 😊
Reverted clang-format patch, and answered inline.
llvm/include/llvm/TargetParser/Triple.h | ||
---|---|---|
163 | IIRC it I did a clang-format because the buildbot complained the format was not right. | |
llvm/unittests/TargetParser/TripleTest.cpp | ||
1289 | This is a very valid question! And I'm not sure of the best option. But the options I had didn't seem to allow this kind of "undefined architecture". |
llvm/include/llvm/TargetParser/Triple.h | ||
---|---|---|
163 | I know what you did and it makes sense - I did the same myself while working on another backend but lets try not to change such a large amount of lines in a patch focused on something else. I think it's certainly better to propose this change in a single NFC patch if we think that clang-format output is better than the existing formatting. | |
llvm/unittests/TargetParser/TripleTest.cpp | ||
1289 | 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. |
llvm/include/llvm/TargetParser/Triple.h | ||
---|---|---|
163 | Of course. So is the presubmit failure something we can ignore? Or shall I do another NFC patch to the format? |
llvm/unittests/TargetParser/TripleTest.cpp | ||
---|---|---|
1289 | 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. |
llvm/unittests/TargetParser/TripleTest.cpp | ||
---|---|---|
1289 | Hello! Thanks for the feedback! We also have the SPV_KHR_variable_pointers extension (https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/KHR/SPV_KHR_variable_pointers.asciidoc)
Not familiar enough with LLVM/Clang to fully understand, so reformulating we'd have:
If that's the case, seems fair, I'd follow this advice.
Same idea here, add VulkanX to the OSType part of the triple (replacing the ShaderModel used for DXIL). |
llvm/unittests/TargetParser/TripleTest.cpp | ||
---|---|---|
1289 | 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. |
Changed the pointer size to 32 bit to 64 bits.
So far, I haven't see any codegen difference, as logical pointers do
not have a fixed size (driver will set it).
However, in case of a raw buffer load (Vulkan extension), with the
PhysicalStorageBuffer64 memory model, pointers are fixed to 64bits.
Since I haven't found a competing memory model with a fixed 32-bit pointer
size in use for Vulkan, I thought we had 1 argument to choose 64 bits.
But once again, this shouldn't change anything, as pointers have no size.
Commits for this change:
- fixup! [SPIR-V] Add SPIR-V logical triple.
@Keenuts Hi Nathan, thanks for the patch! I agree with your approach and I think that this solution despite being a "hack" seems to be the most straightforward. Eventual differences could be handled in the backend. I would be open for other solutions in the future (for example in case other targets would also stumble upon a similar dilemma), but this looks like a completely valid approach to me.
@Anastasia Thanks for your input on this thread! I tried reaching out to you, but I realized you may no longer have access to the email address.
LGTM!
Thanks!
Do you know how we can get this merged? (we have no commit access on our end).
Either I could push the patch for you (adding you as an author) or preferably you could gain commit access yourself for this and future patches. Please see this guide and send an email to Chris (include your GitHub user name and a link to this Phabricator review). Please let me know which option you would prefer.
Either I could push the patch for you (adding you as an author) or preferably you could gain commit access yourself for this and future patches.
Thanks, got commit access. Running tests again and merging this.
No need to reindent the whole block to add a single line.