This is an archive of the discontinued LLVM Phabricator instance.

[SPIRV] Add SPIR-V logical triple.
ClosedPublic

Authored by Keenuts on Jul 21 2023, 9:14 AM.

Details

Summary

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

Diff Detail

Event Timeline

Keenuts created this revision.Jul 21 2023, 9:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2023, 9:14 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Keenuts updated this revision to Diff 544701.Jul 27 2023, 4:24 AM

Add the SPIRV define in all SPIRV targets.

Keenuts updated this revision to Diff 544703.Jul 27 2023, 4:27 AM

format fix & rebase

Keenuts updated this revision to Diff 547682.Aug 7 2023, 2:40 AM

rebasing

Keenuts published this revision for review.Aug 7 2023, 4:46 AM
Keenuts edited the summary of this revision. (Show Details)
Keenuts added reviewers: Anastasia, mpaszkowski.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 7 2023, 4:46 AM
Keenuts updated this revision to Diff 547722.EditedAug 7 2023, 4:50 AM

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.

Keenuts retitled this revision from [SPIR-V] Add SPIR-V logical triple. to [SPIRV] Add SPIR-V logical triple..Aug 10 2023, 12:43 PM

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.

453

Same as above. I guess there's no need to reindent a whole block.

604

Whole block reindent.

llvm/unittests/TargetParser/TripleTest.cpp
1307

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?

Keenuts updated this revision to Diff 549288.Aug 11 2023, 1:38 AM

Revert "clang-format"

Keenuts marked 4 inline comments as done.Aug 11 2023, 1:45 AM

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.
Reverted the clang-format commit (which seems better, I agree), and I'll see if the bots complains.

llvm/unittests/TargetParser/TripleTest.cpp
1307

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.

pmatos added inline comments.Aug 11 2023, 4:47 AM
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
1307

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.

Keenuts marked an inline comment as done.Aug 11 2023, 5:29 AM
Keenuts added inline comments.
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?

Anastasia added inline comments.Aug 11 2023, 10:16 AM
llvm/unittests/TargetParser/TripleTest.cpp
1307

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.

Keenuts added inline comments.Aug 14 2023, 7:39 AM
llvm/unittests/TargetParser/TripleTest.cpp
1307

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

Keenuts added inline comments.Aug 23 2023, 9:50 AM
llvm/unittests/TargetParser/TripleTest.cpp
1307

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.

dschuff removed a subscriber: dschuff.Aug 23 2023, 11:38 AM
Keenuts updated this revision to Diff 555745.Sep 4 2023, 6:44 AM

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.
mpaszkowski accepted this revision.Sep 5 2023, 4:35 PM

@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!

This revision is now accepted and ready to land.Sep 5 2023, 4:35 PM

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

Thanks!
Do you know how we can get this merged? (we have no commit access on our end).

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

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.

This revision was landed with ongoing or failed builds.Sep 11 2023, 1:15 AM
This revision was automatically updated to reflect the committed changes.