search

intel / llvm

Intel staging area for llvm.org contribution. Home for Intel LLVM-based projects.
Other
1.21k stars 725 forks source link

Handling of Builtin Variables violates OpenCL 3.0 SPIR-V Environment Specification. #6703

Open karolherbst opened 2 years ago

karolherbst commented 2 years ago

Describe the bug The OpenCL 3.0 SPIR-V Environment specifies under 2.9. Built-in Variables that all builtin variables have to be in the Input Storage Class:

"An OpVariable in a SPIR-V module with the BuiltIn decoration represents a built-in variable. All built-in variables must be in the Input storage class."

But the generated sycl SPIR-V puts them into the Cross Workgroup Storage class.

This is a new requirement introduced by OpenCL 3.0, though earlier versions did not specify details of BuiltIn variables, using Cross Workgroup doesn't make any sense from a technical perspective either way.

To Reproduce This can be seen with any SyCL program using builtins.

  1. fetch the sycl sample app from https://intel.github.io/llvm-docs/GetStartedGuide.html#run-simple-dpc-application
  2. clang++ -fsycl -fsycl-device-only -fno-sycl-use-bitcode -fsycl-targets=spir64-unknown-unknown test.cpp -o - | spirv-dis

relevant bits of the SPIR-V:

               OpDecorate %__spirv_BuiltInGlobalInvocationId LinkageAttributes "__spirv_BuiltInGlobalInvocationId" Import
               OpDecorate %__spirv_BuiltInGlobalInvocationId BuiltIn GlobalInvocationId
%_ptr_CrossWorkgroup_uint = OpTypePointer CrossWorkgroup %uint
%__spirv_BuiltInGlobalInvocationId = OpVariable %_ptr_CrossWorkgroup_v3ulong CrossWorkgroup

Environment:

Additional context

SyCL binaries shouldn't rely on spec violations to be accepted by OpenCL runtimes even though if requirements were introduced later on.

Also, this doesn't happen if upstream clang is used to compile normal OpenCL C applications using get_global_id to SPIR-V through https://github.com/KhronosGroup/SPIRV-LLVM-Translator

karolherbst commented 11 months ago

This is still an issue and sadly it's kinda critical because almost every SyCL binary is hitting this. Any chance that anybody could look at it? Otherwise I might try to dig into it myself and figure out what's going wrong.

asudarsa commented 11 months ago

Hi @karolherbst

We will take a look at this and provide you an update soon. Thanks for your patience.

Sincerely

asudarsa commented 11 months ago

I am able to reproduce the error with the latest intel/llvm compiler.

We have the following code here:

%_ptr_CrossWorkgroup_v3ulong = OpTypePointer CrossWorkgroup %v3ulong
...
%__spirv_BuiltInGlobalInvocationId = OpVariable %_ptr_CrossWorkgroup_v3ulong CrossWorkgroup

SPIR-V unified spec has the following wording here: Storage Class is the Storage Class of the memory holding the object. It must not be Generic. It must be the same as the Storage Class operand of the Result Type.

I can thus see where CrossWorkgroup StorageCalss comes from.

Investigating further...

Thanks

asudarsa commented 6 months ago

Hi @karolherbst

Sorry for break in pursuing this. I am actively looking at this now and provide a resolution. Interestingly, when I compile and run using the latest tools in intel/llvm repo, I am able to see the SPIR-V code snippet as reported in the description. But SPIR-V Tools do not emit any errors for this SPIR-V file. Will provide an update soon.

Thanks

MrSidims commented 6 months ago

But SPIR-V Tools do not emit any errors for this SPIR-V file.

The report says, that generated SPIR-V is not conformant to https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_Env.html . SPIR-V Tools only checks conformance to https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.pdf

asudarsa commented 6 months ago

Not sure about that. SPIR-V Tools do seem to cover OpenCL SPIR-V environment spec as well. Here is a list of issues being tracked there. https://github.com/KhronosGroup/SPIRV-Tools/milestone/6 This issue has also been reported there: https://github.com/KhronosGroup/SPIRV-Tools/issues/3355

asudarsa commented 5 months ago

A fix has been added here: https://github.com/KhronosGroup/SPIRV-LLVM-Translator/pull/2456 It will be pulled into intel/llvm in a timely manner.

Thanks

asudarsa commented 4 months ago

Based on reviews and other feedback, I have concluded that https://github.com/KhronosGroup/SPIRV-LLVM-Translator/pull/2456 is a difficult workaround to maintain and guarantee that it will not break anything else. I will try to come up with a more rounded solution.

Thanks