spirv-val: Validate zero product workgroup size

[spencer-lunarg]

Improved version of #4828

closes #4801

Created a EntryPointData reduce number of hash maps with the entry_point_id as the key

[alan-baker]

Still thinking about the change to save execution mode instructions. I'm not so sure about that.

[s-perron]

@alan-baker Any more thoughts?

[spencer-lunarg]

@alan-baker I found there was a EntryPointLocalSizeOrId added from Coop Matrix and was able to reuse that and didn't need to touch at state tracking at all now.

I redid this PR (force pushed), its in 3 commits now for easier viewing as well

[VyacheslavLevytskyy]

@alan-baker @s-perron I see an issue with this patch - as far as I can judge. It introduces a check for WorkgroupSize variable to be a 3-component 32-bit int vector, and indeed, I see this requirement in https://registry.khronos.org/vulkan/specs/latest/man/html/WorkgroupSize.html#VUID-WorkgroupSize-WorkgroupSize-04427

However, OpenCL imposes different requirements, documented here: https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_Env.html#_built_in_variables
OpenCL environment requires WorkgroupSize variable to have components of size_t size that will be 32 or 64 depending on a target. This is the way how the SPIR-V Backend implements this, by querying pointer size of the current platform/target.

This change in spirv-val leads to fails in four tests cases in SPIR-V Backend (see llvm/llvm-project#122162 (comment)), intended for the OpenCL/SYCL environment. At the same time, those test cases are valid in generating something of the kind

%ulong = OpTypeInt 64 0
%v3ulong = OpTypeVector %ulong 3
%_ptr_CrossWorkgroup_v3ulong = OpTypePointer CrossWorkgroup %v3ulong
%__spirv_BuiltInWorkgroupSize = OpVariable %_ptr_CrossWorkgroup_v3ulong CrossWorkgroup

after invoking llc -mtriple=spirv64v1.5-unknown-unknown ..., according to both SPIR-V Specification and OpenCL SPIR-V Environment Specification.

Please validate my reasoning. What do you think?

[spencer-lunarg]

@VyacheslavLevytskyy happy to help here (just lack OpenCL SPIR-V knowledge), can you show me what the OpenCL looks like that is triggering this (I can't get an error with https://godbolt.org/z/qYo61or9z)

[VyacheslavLevytskyy]

@VyacheslavLevytskyy happy to help here (just lack OpenCL SPIR-V knowledge), can you show me what the OpenCL looks like that is triggering this (I can't get an error with https://godbolt.org/z/qYo61or9z)

Thank you! Please have a look at test cases:

https://github.com/llvm/llvm-project/tree/main/llvm/test/CodeGen/SPIRV/pointers/type-deduce-sycl-stub.ll
https://github.com/llvm/llvm-project/tree/main/llvm/test/CodeGen/SPIRV/transcoding/OpGroupBroadcast.ll
https://github.com/llvm/llvm-project/tree/main/llvm/test/CodeGen/SPIRV/validate/sycl-hier-par-basic.ll
https://github.com/llvm/llvm-project/tree/main/llvm/test/CodeGen/SPIRV/validate/sycl-tangle-group-algorithms.ll

I have no OpenCL equivalent for them.

They would fail after executing

llc -O0 -mtriple=spirv64-unknown-unknown FILE_NAME -o - -filetype=obj | spirv-val

[VyacheslavLevytskyy]

I can't get an error with https://godbolt.org/z/qYo61or9z

Probably there we see still the previous spirv-val version before this changes are applied?

[s-perron]

I think there are 2 things to fix:

1. The llvm test should be setting and opencl target-env:

llc -O0 -mtriple=spirv64-unknown-unknown FILE_NAME -o - -filetype=obj | spirv-val --target-env <env>

2. Second we might need to update the check in spirv-val to not do a different check for OpenCL.

[spencer-lunarg]

Probably there we see still the previous spirv-val version before this changes are applied?

The compiler-explorer uses ToT SPIRV-Tools

Second we might need to update the check in spirv-val to not do a different check for OpenCL.

This should probably be an issue, and someone with the knowledge of the nuances how OpenCL does workgroup size check should either do it, or at least give me some sample SPIR-V to add as test cases

[VyacheslavLevytskyy]

@s-perron I created llvm/llvm-project#122755
As for the fix here, it looks like just checking for Vulkan env instead of general check without env specification should to the trick.

[VyacheslavLevytskyy]

@spencer-lunarg Here the sample SPIR-V

; SPIR-V
; Version: 1.4
; Generator: LLVM LLVM SPIR-V Backend; 20
; Bound: 27
; Schema: 0
               OpCapability Kernel
               OpCapability Addresses
               OpCapability Int64
               OpCapability Linkage
          %1 = OpExtInstImport "OpenCL.std"
               OpMemoryModel Physical64 OpenCL
               OpEntryPoint Kernel %test "test" %__spirv_BuiltInWorkgroupSize
               OpExecutionMode %test ContractionOff
               OpSource OpenCL_CPP 100000
               OpName %test "test"
               OpName %__spirv_BuiltInWorkgroupSize "__spirv_BuiltInWorkgroupSize"
               OpName %entry "entry"
               OpDecorate %__spirv_BuiltInWorkgroupSize Constant
               OpDecorate %__spirv_BuiltInWorkgroupSize LinkageAttributes "__spirv_BuiltInWorkgroupSize" Import
               OpDecorate %__spirv_BuiltInWorkgroupSize BuiltIn WorkgroupSize
       %void = OpTypeVoid
      %ulong = OpTypeInt 64 0
       %uint = OpTypeInt 32 0
    %v3ulong = OpTypeVector %ulong 3
%_ptr_Input_v3ulong = OpTypePointer Input %v3ulong
          %8 = OpTypeFunction %void
%__spirv_BuiltInWorkgroupSize = OpVariable %_ptr_Input_v3ulong Input
       %test = OpFunction %void None %8
      %entry = OpLabel
         %11 = OpLoad %v3ulong %__spirv_BuiltInWorkgroupSize Aligned 1
         %12 = OpCompositeExtract %ulong %11 0
               OpReturn
               OpFunctionEnd

[VyacheslavLevytskyy]

@spencer-lunarg Was my example helpful? Please let me know if I can help anyhow.

Also if you can please estimate how soon we may get the resolution of this problem this would help me to decide if we need to wait for the fix a bit or to mark 4 failing test cases as XFAIL right now.

[spencer-lunarg]

@VyacheslavLevytskyy example was good, got a fix up an hour ago - #5940

[VyacheslavLevytskyy]

@VyacheslavLevytskyy example was good, got a fix up an hour ago - #5940

thank you!