This is an archive of the discontinued LLVM Phabricator instance.

[SPIRV] Add SPIR-V logical triple to llc
ClosedPublic

Authored by Keenuts on Jul 27 2023, 5:46 AM.

Details

Summary

This commits adds the minimal required bits to build a logical SPIR-V
compute shader using LLC.

  • Skip OpenCL-only capabilities & extensions for Logical SPIR-V.
  • Checks hlsl.numthreads attribute emitted by the HLSL frontend.
  • Fix execution mode to GLCompute in logical.

The main issue is the lack of "vulkan" bit in the triple.
Because as-is, SPIRV32/64 assumes OpenCL, and then, SPIRV assumes
Vulkan. This is ok-ish today, but not correct.

Idea is to replace the "shadermodelX" part the triple with "vulkanX". But this needs some thinking.

Depends on D155978

Diff Detail

Event Timeline

Keenuts created this revision.Jul 27 2023, 5:46 AM
Keenuts updated this revision to Diff 544729.Jul 27 2023, 5:49 AM

git process crashed, no changes I suppose.

iliya-diyachkov retitled this revision from [SPIRV-V] Add SPIR-V logical triple to llc to [SPIRV] Add SPIR-V logical triple to llc.Jul 27 2023, 12:59 PM
Keenuts updated this revision to Diff 549363.Aug 11 2023, 5:59 AM
  • [SPIRV-V] Add SPIR-V logical triple to llc
  • fixup! [SPIRV-V] Add SPIR-V logical triple to llc
  • fixup! [SPIRV-V] Add SPIR-V logical triple to llc
  • fixup! [SPIRV-V] Add SPIR-V logical triple to llc

Removed the Logical SPIRV data layout string. We might want to
stick with SPIRV32 data layout while this is fuzzy.

Removed changes to the HLSL frontend, and adds logic to check
the HLSL attributes in the backend.
I believe this is better, as we can emit LLVM from HLSL without having
to know the target backend.
Note: we might want to unify attributes later.

Keenuts published this revision for review.Aug 11 2023, 6:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2023, 6:06 AM
Keenuts added a subscriber: beanz.Aug 11 2023, 6:06 AM

I believe this should be ready for review.
No mention of Vulkan in the triple yet (we do like SPIRV32/64, assume the API from the arch).
Next step would be to extend the triple to contain Vulkan (idea is to replace shadermodelX with vulkanX, need to discuss this with @beanz )

Keenuts edited the summary of this revision. (Show Details)Aug 11 2023, 6:07 AM
beanz added a subscriber: bogner.

Next step would be to extend the triple to contain Vulkan (idea is to replace shadermodelX with vulkanX, need to discuss this with @beanz )

vulkanX in the triple makes sense to me.

Looping in @bogner.

bogner accepted this revision.EditedAug 17 2023, 9:39 AM

This looks like a good start. I think a fair amount of it will need to change as the HLSL support matures, but most of that should be fine to handle in tree. Probably needs an approval from somebody who's worked more in the SPIR-V backend than I have as well though.

llvm/lib/Target/SPIRV/SPIRVAsmPrinter.cpp
436

Not a problem with your patch in particular, but we need to change how we represent numthreads to something more sensible. Having every user of it parsing this string is ridiculous.

llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
249

These both look reasonable for now, though as you mention we need to discuss unifying how we represent this stuff

llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp
539

Is this is a behaviour change? Looks like we'd just carry on without any capabilities before, but isOpenCLEnv() always returned true so maybe it was just impossible.

932

Does this do something reasonable if both "reqd_work_group_size" and "hlsl.numthreads" are set? By reasonable I mean something like "error" or "accept them if they're compatible", as opposed to "choose one arbitrarily". Might become irrelevant if we're able to unify the representation.

llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
42

Can we get this from the datalayout at this point? It'd be nice to not have to update this in two places when we revisit it.

llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp
49–53

Probably need a TODO here instead of or in addition to the one in SPIRVSubtarget.cpp's computePointerSize

This revision is now accepted and ready to land.Aug 17 2023, 9:39 AM
Keenuts updated this revision to Diff 553088.Aug 24 2023, 5:53 AM
Keenuts marked 2 inline comments as done.
  • fixup! [SPIR-V] Add SPIR-V logical triple.
  • fixup! [SPIR-V] Add SPIR-V logical triple.
Keenuts updated this revision to Diff 553091.Aug 24 2023, 6:02 AM
  • fixup! [SPIR-V] Add SPIR-V logical triple.

Thanks for the review. Addressed the feedback. Waiting on https://reviews.llvm.org/D155978 to be merged before rebasing.

llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp
539

Yes, it is a behavior change, but was unreachable. (isOpenCLEnv() was as you said, always true).
Now that the value can be something else, I thought a noisy failure if we reached an in-between state was probably better.

932

Good catch.
Right now, we emit both, as this is valid SPIR-V, but the order depends of the order of those conditions, which makes is I believe not useful at all, just weird.
Adding an error in such cases, as I don't see a valid use-case for this.

llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
42

Yes, that's better. Removed this in favor of getting it from the DataLayout (via TargetMachine)

llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp
49–53

Done. Removed computePointerSize since we get it from datalayout, and moved the TODO here.

Keenuts updated this revision to Diff 556045.Sep 6 2023, 8:14 AM

Changed the pointer size from 32 bits to 64 bits to match
the parent change.

Reason: I had to pick one. 64 bits might allow us to implement
PhysicalStorage64 memory model with more ease, but this should have
no impact for Logical SPIR-V.

iliya-diyachkov added inline comments.Sep 7 2023, 1:32 PM
llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
216–218

Maybe rename it to isEntryPoint? Also "entrypoint" -> "entry point"?

219–221

Could we avoid braces?

225–227

Braces?

234–236

Braces?

llvm/lib/Target/SPIRV/SPIRVSubtarget.cpp
79–81

Braces?

Keenuts updated this revision to Diff 556239.Sep 8 2023, 2:20 AM

coding style fixes.

Keenuts updated this revision to Diff 556240.Sep 8 2023, 2:21 AM

typo fix

Keenuts marked 5 inline comments as done.Sep 8 2023, 2:22 AM

Thanks, coding-style fixes applied.

iliya-diyachkov accepted this revision.EditedSep 8 2023, 5:14 AM

Thanks, Nathan.

The patch looks good to me.

This revision was automatically updated to reflect the committed changes.