[SPIR-V] DRAFT: Shader built-ins - no spec change #116393
Conversation
Draft PR to explore adding semantics & inline SPIR-V for builtins. This PR is an alternative to llvm#115187 which does not requires a spec change as we keep the variables as `static [const]`. In addition, it tried to remain closer to DXIL by using a load/store builtin intrinsic instead of relying on an external global variable (IR level). ``` // RUN: %clang --driver-mode=dxc -T cs_6_6 -spirv %s -O3 -E main [[vk::ext_builtin_input(/* NumWorkGroups */ 24)]] static const uint3 input; [[vk::ext_builtin_output(/* Random */ 25)]] static uint3 output; [shader("compute")] [numthreads(32, 1, 1)] void main(uint3 id : SV_GroupID, uint3 id2 : SV_GroupID) { output = input + id2 + id; } ``` Note: this code wouldn't pass validation because the output built-in is invalid. Signed-off-by: Nathan Gauër <[email protected]>
| case 8: | ||
| return SPIRV::StorageClass::Input; |
There was a problem hiding this comment.
Are you sure you can change the input storage class's address space?
There was a problem hiding this comment.
No, that's something I need to clear with Intel regarding the LLVM SPIR-V translator.
Right now, this is not used, so I don't know if that's something added but never used, or if that's related to LLVM translator handling of those address space.
(This can change, as we see in the Discourse threads about AS numbering change, but if it's used in LLVM translator, I'd need to do it everywhere (in such case I wouldn't change it, but keep the same number)
Anyway, the actual value is not important
| if (D.hasAttr<HLSLSV_GroupIDAttr>()) { | ||
| if (getArch() == llvm::Triple::spirv) { | ||
| llvm::Type *Ty = CGM.getTypes().ConvertTypeForMem(D.getType()); | ||
| llvm::Function *IntrinsicID = | ||
| CGM.getIntrinsic(Intrinsic::spv_load_builtin, {Ty}); | ||
| return B.CreateCall(IntrinsicID, {B.getInt32(/* WorkgroupID */ 26)}); | ||
| } else | ||
| // FIXME: getIntrinsic(getGroupIDIntrinsic()) | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
Could something be added to CommonSPIRTargetCodeGenInfo and the DXIL equivalent? That way the spir-v specific ids will be hidden in a spirv specific class.
There was a problem hiding this comment.
Yes, this can clearly be refactorized in a real implementation. Just wanted to keep it simple for the draft.
| // Vulkan specific address spaces. | ||
| vulkan_private, | ||
|
|
There was a problem hiding this comment.
Do we need a new address space? This is not an issue specific to these input and output variable. We need to figure out how to handle general static variables. There are some tests that already consider this, but they are DXIL only.
We might need to figure that out first, and than have this PR build on top of that. For DXIL, the static variables are marked as "internal global", and does not use a different address space. Can we do the same? Is the DXIL codegen their real solution or just a temporary implementation?
There was a problem hiding this comment.
My understanding is we have 2 ways to convey this information from the FE to the BE:
- using a common IR representation (like
internal global [const]) - using a custom address space.
Right now, the BE considers an "internal global" to have the "Function" storage class, which I believe is wrong. So we could say internal global are Private.
If for some reasons Intel is against this, we can rely on a custom address space.
The less-invasive change is indeed to use the internal global. Which can also be done (Change in SPIRVInstructionSelector.cpp, line 3423 and 3344).
Changing the it doesn't break any tests.
There was a problem hiding this comment.
It breaks 1 test, but the test seems wrong anyway: test for SPV_INTEL_function_pointers, which is not official IIRC (could only find https://github.com/intel/llvm/blob/sycl/sycl/doc/design/spirv-extensions/SPV_INTEL_function_pointers.asciidoc ) and the spec seems to require the SC to be CodeSectionINTEL and the test expects it to be Function.
Anyway, if MS approves the overall direction to go through this Ctor/Dtor, I'll make a real PR with those changes, and not use a custom AS
| def HLSLOutputBuiltin : SubsetSubject<Var, | ||
| [{S->hasGlobalStorage() && !S->getType().isConstQualified() && | ||
| S->getStorageClass()==StorageClass::SC_Static}], | ||
| "static const global variables">; |
There was a problem hiding this comment.
| "static const global variables">; | |
| "static global variables">; |
Draft PR to explore adding semantics & inline SPIR-V for builtins. This PR is an alternative to #115187 which does not requires a spec change as we keep the variables as
static [const].In addition, it tried to remain closer to DXIL by using a load/store builtin intrinsic instead of relying on an external global variable (IR level).
Note: this code wouldn't pass validation because the output built-in is invalid.