Headers support for two Intel extensions #356

vmaksimo · 2023-06-30T12:20:06Z

Add SPV_INTEL_global_variable_fpga_decorations
Spec: https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/INTEL/SPV_INTEL_global_variable_fpga_decorations.asciidoc
Add SPV_INTEL_global_variable_host_access
Spec: https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/INTEL/SPV_INTEL_global_variable_host_access.asciidoc

Spec: https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/INTEL/SPV_INTEL_global_variable_fpga_decorations.asciidoc

Spec: https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/INTEL/SPV_INTEL_global_variable_host_access.asciidoc

artemrad

Looks good. My only concern is that it doesn't remove any of the legacy implementations for these decorations. I can't seem to find any reference to them either, so I am a bit confused.

vmaksimo · 2023-06-30T14:39:58Z

@artemrad legacy implementation was done in SPIRV-LLVM Translator only according to https://github.com/intel/llvm/blob/sycl/sycl/doc/design/spirv-extensions/SPV_INTEL_global_variable_decorations.asciidoc spec.
Once we merge these new headers, we'll be able to switch to the official extension and remove old decorations in llvm-spirv.

vmaksimo · 2023-07-06T15:04:05Z

@MrSidims @bashbaug please take a look. Thanks!

bashbaug · 2023-07-07T20:26:43Z

include/spirv/unified1/spirv.core.grammar.json

@@ -11880,6 +11880,32 @@
        }
      ]
    },
+    {
+      "category" : "ValueEnum",
+      "kind" : "HostAccessQualifier",


These new enums for HostAccessQualifier and the enums for InitializationModeQualifier aren't getting added to the headers because the generation scripts don't know about them. Are we OK with that?

Actually, I'm a little surprised the header generation script ran correctly because it has checks for unhandled "operand kinds":

if (type == OperandNone) { std::cerr << "Unhandled operand kind found: " << operandKind << std::endl; exit(1); }

Did the generation script run correctly or were other changes needed?

I took a look at this in a bit more detail because all of the automated checks are passing. It turns out that this error is certainly occurring, though it isn't causing the CI jobs to fail for some reason. We should fix this.

See: https://github.com/KhronosGroup/SPIRV-Headers/actions/runs/5423707347/jobs/9931446730?pr=356#step:7:15

Run cd tools/buildHeaders dos2unix: converting file DebugInfo.h to Unix format... dos2unix: converting file OpenCLDebugInfo100.h to Unix format... dos2unix: converting file AMD_gcn_shader.h to Unix format... dos2unix: converting file AMD_shader_ballot.h to Unix format... dos2unix: converting file AMD_shader_explicit_vertex_parameter.h to Unix format... dos2unix: converting file AMD_shader_trinary_minmax.h to Unix format... dos2unix: converting file NonSemanticDebugPrintf.h to Unix format... dos2unix: converting file NonSemanticClspvReflection.h to Unix format... dos2unix: converting file NonSemanticDebugBreak.h to Unix format... Unhandled operand kind found: InitializationModeQualifier

@bashbaug thanks for review and investigation! What do you think, should I add these enums to the generation script or to add them somewhere in different place?

should I add these enums to the generation script

Yes please. This shouldn't be too bad. You can have a look at the recent cooperative matrix PR #355 as an example since it also added a few new types (see the changes in tools/buildHeaders).

Once this is done I would also recommend checking that a basic test case disassembles and assembles properly since updates may be needed to SPIRV-Tools also.

Thanks!

Hi @bashbaug! I've updated the headers generation. However I'm not sure how to test SPIR-V tools, because we don't have implementation for these headers in llvm-spirv yet. The CI checks are passing, could it be enough? Thanks!

Can you please check that SPIRV-Tools builds with these changes? That would probably be the minimum to check, since the interaction between SPIRV-Headers and SPIRV-Tools is not checked via CI.

I'd recommend doing a light touch-test to see if a SPIR-V module using the new decorations assembles, disassembles, and validates properly also, though if this is broken it'll only affect the users of these extensions but not everyone using SPIR-V. You could do this manually, say by adding the decoration to a small existing SPIR-V ASM file, if the tooling to generate SPIR-V modules using these extensions is not implemented yet.

I tried building SPIRV-Tools with these header changes just to see if it'd work and I hit build errors. In case it's helpful:

In file included from /home/bashbaug/git/spirv-tools/source/operand.cpp:36: /home/bashbaug/git/spirv-tools/build/operand.kinds-unified1.inc:871:99: error: ‘SPV_OPERAND_TYPE_INITIALIZATION_MODE_QUALIFIER’ was not declared in this scope; did you mean ‘SPV_OPERAND_TYPE_OPTIONAL_ACCESS_QUALIFIER’? 871 | {"InitModeINTEL", 6147, 1, pygen_variable_caps_GlobalVariableFPGADecorationsINTEL, 0, nullptr, {SPV_OPERAND_TYPE_INITIALIZATION_MODE_QUALIFIER}, 0xffffffffu, 0xffffffffu}, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | SPV_OPERAND_TYPE_OPTIONAL_ACCESS_QUALIFIER /home/bashbaug/git/spirv-tools/build/operand.kinds-unified1.inc:873:96: error: ‘SPV_OPERAND_TYPE_HOST_ACCESS_QUALIFIER’ was not declared in this scope; did you mean ‘SPV_OPERAND_TYPE_ACCESS_QUALIFIER’? 873 | {"HostAccessINTEL", 6168, 1, pygen_variable_caps_GlobalVariableHostAccessINTEL, 0, nullptr, {SPV_OPERAND_TYPE_HOST_ACCESS_QUALIFIER, SPV_OPERAND_TYPE_LITERAL_STRING}, 0xffffffffu, 0xffffffffu}, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | SPV_OPERAND_TYPE_ACCESS_QUALIFIER

Thanks Ben! I've also reproduced it locally, and just forgot to upload my fixes to the Tools - you can see the PR in the reference now :)
Now tools are buildable, local ctests pass, and the "basic" spirv with added capability and extension are going without problems through spirv-val, spirv-as and spirv-dis.

* SPV_INTEL_global_variable_fpga_decorations Spec: https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/INTEL/SPV_INTEL_global_variable_fpga_decorations.asciidoc * SPV_INTEL_global_variable_host_access Spec: https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/INTEL/SPV_INTEL_global_variable_host_access.asciidoc This change follows headers change: KhronosGroup/SPIRV-Headers#356

bashbaug · 2023-08-10T21:56:44Z

@alan-baker can you please confirm it's OK to merge this? Thanks!

alan-baker

Seems ok

* SPV_INTEL_global_variable_fpga_decorations Spec: https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/INTEL/SPV_INTEL_global_variable_fpga_decorations.asciidoc * SPV_INTEL_global_variable_host_access Spec: https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/INTEL/SPV_INTEL_global_variable_host_access.asciidoc This change follows headers change: KhronosGroup/SPIRV-Headers#356

bashbaug

Merging as discussed in the August 16th teleconference.

vmaksimo added 2 commits June 30, 2023 05:17

Add SPV_INTEL_global_variable_fpga_decorations

0688b3a

Spec: https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/INTEL/SPV_INTEL_global_variable_fpga_decorations.asciidoc

Add SPV_INTEL_global_variable_host_access

94e6cb7

Spec: https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/INTEL/SPV_INTEL_global_variable_host_access.asciidoc

vmaksimo force-pushed the add-device-globals branch from 777e108 to 94e6cb7 Compare June 30, 2023 13:59

vmaksimo marked this pull request as ready for review June 30, 2023 14:00

artemrad reviewed Jun 30, 2023

View reviewed changes

bashbaug reviewed Jul 7, 2023

View reviewed changes

vmaksimo added 3 commits July 19, 2023 08:29

Update headers generator

be63185

update headers after generating script

54e7583

Merge remote-tracking branch 'origin/main' into add-device-globals

753d736

vmaksimo mentioned this pull request Aug 2, 2023

Support 2 Intel extensions KhronosGroup/SPIRV-Tools#5357

Merged

vmaksimo requested a review from bashbaug August 7, 2023 14:25

bashbaug requested a review from alan-baker August 10, 2023 21:56

alan-baker approved these changes Aug 11, 2023

View reviewed changes

bashbaug approved these changes Aug 16, 2023

View reviewed changes

bashbaug merged commit b8b9eb8 into KhronosGroup:main Aug 16, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Headers support for two Intel extensions #356

Headers support for two Intel extensions #356

vmaksimo commented Jun 30, 2023

artemrad left a comment

vmaksimo commented Jun 30, 2023

vmaksimo commented Jul 6, 2023

bashbaug Jul 7, 2023

bashbaug Jul 7, 2023

vmaksimo Jul 18, 2023

bashbaug Jul 18, 2023

vmaksimo Jul 25, 2023

bashbaug Jul 25, 2023

bashbaug Aug 1, 2023

vmaksimo Aug 2, 2023

bashbaug commented Aug 10, 2023

alan-baker left a comment

bashbaug left a comment

Headers support for two Intel extensions #356

Headers support for two Intel extensions #356

Conversation

vmaksimo commented Jun 30, 2023

artemrad left a comment

Choose a reason for hiding this comment

vmaksimo commented Jun 30, 2023

vmaksimo commented Jul 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bashbaug commented Aug 10, 2023

alan-baker left a comment

Choose a reason for hiding this comment

bashbaug left a comment

Choose a reason for hiding this comment