llvm icon indicating copy to clipboard operation
llvm copied to clipboard

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

Open karolherbst opened this issue 3 years ago • 8 comments

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:

  • OS: Linux
  • DPC++ version: clang version 16.0.0 (https://github.com/intel/llvm 22e3fc56c8c2bef6d92a19819b7bea106a7992d1)

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 avatar Sep 05 '22 21:09 karolherbst

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.

karolherbst avatar Oct 12 '23 18:10 karolherbst

Hi @karolherbst

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

Sincerely

asudarsa avatar Oct 12 '23 18:10 asudarsa

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 avatar Oct 12 '23 19:10 asudarsa

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

asudarsa avatar Feb 15 '24 00:02 asudarsa

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

MrSidims avatar Feb 15 '24 10:02 MrSidims

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 avatar Feb 17 '24 04:02 asudarsa

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 avatar Mar 22 '24 03:03 asudarsa

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

asudarsa avatar May 06 '24 14:05 asudarsa