Make storage class of builtins to be Input #2456

asudarsa · 2024-03-22T03:15:45Z

In case of SPIRV builtin global variables, the storage class function should be StorageClassInput, irrespective of the incoming addrspace.
Ref: https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_Env.html
Sec 2.9. Built-in variables.

Forcing the storage class like this results in illegal casts between SPIRV builtin global variables and their users. To avoid this, the global variables are copied over to local memory during the SPIRVRegularizeLLVM pass.
SPIRV Reader has also been modified to react to these changes appropriately.

Thanks
Sincerely

test/builtin-functions.ll

test/builtin-storage-class.ll

asudarsa · 2024-03-22T05:19:07Z

All the test fails in the CI report this error:
error: line XX: Expected input to have storage class Workgroup, CrossWorkgroup or Function: PtrCastToGeneric
%37 = OpPtrCastToGeneric %_ptr_Generic_uint %__spirv_BuiltInSubgroupMaxSize

This translation is caused by the LLVM IR in the test:
%2 = addrspacecast ptr addrspace(1) @__spirv_BuiltInSubgroupMaxSize to ptr addrspace(4)

This rule contradicts the rule in OpenCL 3.0 SPIR-V env spec shown in the description.
I see that there is a SPIR-V Registry issue that might be related to this.
KhronosGroup/SPIRV-Registry#73

Is it possible to allow Input storage class for OpPtrCastToGeneric?

Thanks
Sincerely

karolherbst · 2024-03-22T09:40:06Z

All the test fails in the CI report this error: error: line XX: Expected input to have storage class Workgroup, CrossWorkgroup or Function: PtrCastToGeneric %37 = OpPtrCastToGeneric %_ptr_Generic_uint %__spirv_BuiltInSubgroupMaxSize

This translation is caused by the LLVM IR in the test: %2 = addrspacecast ptr addrspace(1) @__spirv_BuiltInSubgroupMaxSize to ptr addrspace(4)

This rule contradicts the rule in OpenCL 3.0 SPIR-V env spec shown in the description. I see that there is a SPIR-V Registry issue that might be related to this. KhronosGroup/SPIRV-Registry#73

Is it possible to allow Input storage class for OpPtrCastToGeneric?

I think it would be better to handle this case on the llvm IR level and copy the value to function memory first. Adding fundamental new address spaces to generic is a problem for some hardware, e.g. Nvidia and AMD where they have native support for generic pointers, but only if it's CL global, local, private memory as they can map their local and private memory into the global address space.

If generic pointers get fundamental new address spaces it requires different kind of lowering and more runtime overhead.

Input is also more of a "virtual" address space anyway and it's very hardware specific if the value resides in some accessible memory (e.g. a constant buffer) or if the hardware pulls it via "system value" instructions. In any case, adding Input to generic would make it very painful for runtime compilers to handle in an efficient manner.

asudarsa · 2024-03-26T13:29:49Z

All the test fails in the CI report this error: error: line XX: Expected input to have storage class Workgroup, CrossWorkgroup or Function: PtrCastToGeneric %37 = OpPtrCastToGeneric %_ptr_Generic_uint %__spirv_BuiltInSubgroupMaxSize
This translation is caused by the LLVM IR in the test: %2 = addrspacecast ptr addrspace(1) @__spirv_BuiltInSubgroupMaxSize to ptr addrspace(4)
This rule contradicts the rule in OpenCL 3.0 SPIR-V env spec shown in the description. I see that there is a SPIR-V Registry issue that might be related to this. KhronosGroup/SPIRV-Registry#73
Is it possible to allow Input storage class for OpPtrCastToGeneric?

I think it would be better to handle this case on the llvm IR level and copy the value to function memory first. Adding fundamental new address spaces to generic is a problem for some hardware, e.g. Nvidia and AMD where they have native support for generic pointers, but only if it's CL global, local, private memory as they can map their local and private memory into the global address space.

If generic pointers get fundamental new address spaces it requires different kind of lowering and more runtime overhead.

Input is also more of a "virtual" address space anyway and it's very hardware specific if the value resides in some accessible memory (e.g. a constant buffer) or if the hardware pulls it via "system value" instructions. In any case, adding Input to generic would make it very painful for runtime compilers to handle in an efficient manner.

Thanks @karolherbst
I will add a change to copy this over to function memory.

test/builtin-storage-class.ll

LU-JOHN · 2024-03-29T10:05:14Z

lib/SPIRV/SPIRVWriter.cpp

+// 1. Value associated with the type is a builtin variable.
+// 2. Value associated with the type is a GEPInst with address pointer as a


Are both these cases hit by testing?

After recent changes, we do not need to worry about the second case. The first one is covered.

Thanks

asudarsa · 2024-04-01T21:24:41Z

I am marking this as draft this I resolve the issue with replacing uses of global variable.

Thanks

lib/SPIRV/SPIRVReader.cpp

asudarsa · 2024-04-02T19:46:34Z

lib/SPIRV/SPIRVWriter.cpp

  PassMgr.addPass(SPIRVLowerConstExprPass());
+  PassMgr.addPass(SPIRVRegularizeLLVMPass());


In SPIRVRegularizeLLVMPass, we have some logic where we try to replace a SPIRV Builtin global constant with a variable in local memory. It helps to have the LowerConstExpr pass run before the regularize pass .

Thanks

asudarsa · 2024-04-04T02:13:18Z

Hi @karolherbst, @MrSidims and @LU-JOHN

Can you please take a look at this change when convenient?

Thanks

Signed-off-by: Sudarsanam, Arvind <[email protected]>

LU-JOHN · 2024-04-16T14:30:14Z

test/builtin-storage-class.ll

+; RUN: llvm-spirv %t.bc -spirv-text -o - | FileCheck --check-prefix=CHECK-SPIRV %s
+; RUN: llvm-spirv %t.bc -o %t.spv
+; RUN: spirv-val %t.spv
+


Should we also check reverse translation to exercise the changes in SPIRVReader.cpp?

to react to changes made in forward translation. Signed-off-by: Sudarsanam, Arvind <[email protected]>

Signed-off-by: Sudarsanam, Arvind <[email protected]>

LU-JOHN · 2024-04-16T14:35:27Z

lib/SPIRV/SPIRVReader.cpp

+    auto *StorePtr = SI->getPointerOperand();
+    if (StorePtr->hasName() &&
+        StorePtr->getName().starts_with("__local_SPIRV_Builtin")) {
+      if (auto *LI = dyn_cast<LoadInst>(SI->getValueOperand())) {


Are these if's (at line 1765 and below) necessary? Is it possible that some __local_SPIRV_Builtin's will not be removed in reverse translation?

Good point. They can be turned to 'cast' statements and then asserts to catch any unexpected behavior.

Thanks

MrSidims · 2024-04-17T13:00:38Z

lib/SPIRV/SPIRVRegularizeLLVM.cpp

+/// // Replace uses of __spirv_BuiltInWorkgroupId with %__local_SPIRV_Builtin*
+/// ret void
+/// }
+void SPIRVRegularizeLLVMBase::copyBuiltinGVToLocalVar(Module *M) {


There are also cases, when get_..._id() builtin is represented via function call (both OpenCL and SPIR-V friendly). Are these cases being handled?

yes. I ran into a LIT test fail due to this very same issue and I resolved it.

MrSidims · 2024-04-17T13:01:44Z

lib/SPIRV/SPIRVReader.cpp

+    // (passed via V here). In this case, we also need a AddrSpaceCast to cast
+    // between the new mapped value and the old.
+    if (Loc->second->hasName() &&
+        Loc->second->getName().starts_with("__local_SPIRV_Builtin")) {


If reader counterpart is not done, are functional regressions expected?

I don't think so...I can try it out in a separate PR.

MrSidims · 2024-04-17T13:03:42Z

lib/SPIRV/SPIRVRegularizeLLVM.cpp

+///                               addrspace(1) constant <3 x i64>, align 32
+/// define weak_odr dso_local spir_kernel void @foo() {
+/// ...
+/// %1 = load <3 x i64>, ptr addrspace(1) @__spirv_BuiltInWorkgroupId, align 32


This is not tested

MrSidims · 2024-04-17T13:04:59Z

lib/SPIRV/SPIRVRegularizeLLVM.cpp

+/// %1 = load <3 x i64>, ptr addrspace(1) @__spirv_BuiltInWorkgroupId, align 32
+/// %__local_SPIRV_Builtin* = alloca <3 x i64> addrspace(0), align 32
+/// store <3 x i64> %1, ptr addrspace(0) %__local_SPIRV_Builtin*, align 32
+/// // Replace uses of __spirv_BuiltInWorkgroupId with %__local_SPIRV_Builtin*


What are expected users of __spirv_BuiltInWorkgroupId? Do we need to create alloca and why we can't just use result of the load?

we need to replace one pointer with another.

MrSidims · 2024-04-17T13:05:11Z

lib/SPIRV/SPIRVRegularizeLLVM.cpp

+/// ...
+/// %1 = load <3 x i64>, ptr addrspace(1) @__spirv_BuiltInWorkgroupId, align 32
+/// %__local_SPIRV_Builtin* = alloca <3 x i64> addrspace(0), align 32
+/// store <3 x i64> %1, ptr addrspace(0) %__local_SPIRV_Builtin*, align 32


Shouldn't it be generic address space?

No, the plan here was to copy it to a local variable.

MrSidims · 2024-04-17T13:05:23Z

lib/SPIRV/SPIRVRegularizeLLVM.cpp

+/// ret void
+/// }
+void SPIRVRegularizeLLVMBase::copyBuiltinGVToLocalVar(Module *M) {
+  for (auto I = M->begin(), E = M->end(); I != E;) {


This functionality is not tested

MrSidims · 2024-04-17T13:06:53Z

lib/SPIRV/SPIRVWriter.cpp

+// https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_Env.html
+// Sec 2.9. Built-in variables.
+// Do not store the translated type into PointeeTypeMap for IsBuiltin = true.
+SPIRVType *LLVMToSPIRVBase::transPointerType(Type *ET, unsigned AddrSpc,


Instead of adding extra parameter to the API, can we figure out something like:
IF translate builtin GV
THEN don't call transType, but create Type
?

MrSidims · 2024-04-17T13:12:02Z

lib/SPIRV/SPIRVRegularizeLLVM.cpp

+/// define weak_odr dso_local spir_kernel void @foo() {
+/// ...
+/// %1 = load <3 x i64>, ptr addrspace(1) @__spirv_BuiltInWorkgroupId, align 32
+/// %__local_SPIRV_Builtin* = alloca <3 x i64> addrspace(0), align 32


Also, while there were some recent improvements in instruction ordering in the generated module, still, can we ensure, that OpVariable is placed in the very beginning of the first basic block? Otherwise this validation rule is being violated:
All OpVariable instructions in a function must be in the first block in the function. These instructions, together with any intermixed OpLine and OpNoLine instructions, must be the first instructions in that block. (Note the validation rules prevent OpPhi instructions in the first block of a function.)

I do not explicitly control where the OpVariable defined. in all my testing, I do see it occurring at the top.

MrSidims · 2024-04-17T13:21:09Z

lib/SPIRV/SPIRVRegularizeLLVM.cpp

+            auto *LoadedVal = Builder.CreateLoad(LoadTy, GV);
+            if (GV->getAlignment())
+              LoadedVal->setAlignment(llvm::Align(GV->getAlignment()));
+            auto *NewVar = Builder.CreateAlloca(GV->getType(), nullptr,


We might be having several builtins accessed in the kernel (global id, local id, size etc), if I read the code correctly it would result in several __local_SPIRV_Builtin declarations thus SSA violation. Regardless how it's addressed, please reflect it in the test.

I think it will create them with suffixes.

MrSidims · 2024-04-17T13:24:01Z

lib/SPIRV/SPIRVWriter.cpp

@@ -670,7 +681,8 @@ SPIRVType *LLVMToSPIRVBase::transPointerType(Type *ET, unsigned AddrSpc) {

    auto SaveType = [&](SPIRVType *MappedTy) {
      OpaqueStructMap[Key] = MappedTy;
-      PointeeTypeMap[TypeKey] = MappedTy;
+      if (!IsBuiltin)
+        PointeeTypeMap[TypeKey] = MappedTy;


Does it mean, that for every builtin GV translated we won't visit the map, but instead would translate a type from scratch?

unfortunately ... yes.

asudarsa mentioned this pull request Mar 22, 2024

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

Open

asudarsa requested review from MrSidims and vmaksimo March 22, 2024 03:17

asudarsa commented Mar 22, 2024

View reviewed changes

test/builtin-functions.ll Show resolved Hide resolved

asudarsa commented Mar 22, 2024

View reviewed changes

test/builtin-storage-class.ll Show resolved Hide resolved

LU-JOHN reviewed Mar 29, 2024

View reviewed changes

test/builtin-storage-class.ll Show resolved Hide resolved

LU-JOHN reviewed Mar 29, 2024

View reviewed changes

asudarsa marked this pull request as draft April 1, 2024 21:23

asudarsa commented Apr 2, 2024

View reviewed changes

lib/SPIRV/SPIRVReader.cpp Show resolved Hide resolved

asudarsa marked this pull request as ready for review April 2, 2024 19:05

asudarsa commented Apr 2, 2024

View reviewed changes

asudarsa requested review from maarquitos14, VyacheslavLevytskyy, bwlodarcz and maksimsab April 11, 2024 14:53

Make storage class of builtins to be Input

cd0dca2

Signed-off-by: Sudarsanam, Arvind <[email protected]>

LU-JOHN reviewed Apr 16, 2024

View reviewed changes

asudarsa added 5 commits April 16, 2024 07:33

Copy SPIRV Builtin variable to local memory and also update SPIRV Reader

2db6752

to react to changes made in forward translation. Signed-off-by: Sudarsanam, Arvind <[email protected]>

Address clang-tidy issues

362b493

Signed-off-by: Sudarsanam, Arvind <[email protected]>

Resolve test failures

be6329d

Signed-off-by: Sudarsanam, Arvind <[email protected]>

Fix another clang-tidy issue

1cac9d7

Signed-off-by: Sudarsanam, Arvind <[email protected]>

Possible change

d1a6feb

Signed-off-by: Sudarsanam, Arvind <[email protected]>

LU-JOHN reviewed Apr 16, 2024

View reviewed changes

asudarsa force-pushed the fix_builtin_storage_class branch from c6ef1ef to d1a6feb Compare April 16, 2024 14:36

MrSidims reviewed Apr 17, 2024

View reviewed changes

asudarsa marked this pull request as draft May 6, 2024 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make storage class of builtins to be Input #2456

Make storage class of builtins to be Input #2456

asudarsa commented Mar 22, 2024 •

edited

Loading

asudarsa commented Mar 22, 2024

karolherbst commented Mar 22, 2024

asudarsa commented Mar 26, 2024

LU-JOHN Mar 29, 2024

asudarsa Mar 30, 2024

asudarsa commented Apr 1, 2024

asudarsa Apr 2, 2024

asudarsa commented Apr 4, 2024

LU-JOHN Apr 16, 2024

LU-JOHN Apr 16, 2024 •

edited

Loading

asudarsa Apr 17, 2024

MrSidims Apr 17, 2024 •

edited

Loading

asudarsa Apr 17, 2024

MrSidims Apr 17, 2024 •

edited

Loading

asudarsa Apr 17, 2024

MrSidims Apr 17, 2024

MrSidims Apr 17, 2024

asudarsa Apr 17, 2024

MrSidims Apr 17, 2024

asudarsa Apr 17, 2024

MrSidims Apr 17, 2024

MrSidims Apr 17, 2024

MrSidims Apr 17, 2024 •

edited

Loading

asudarsa Apr 17, 2024

MrSidims Apr 17, 2024

asudarsa Apr 17, 2024

MrSidims Apr 17, 2024

asudarsa Apr 17, 2024

		// 1. Value associated with the type is a builtin variable.
		// 2. Value associated with the type is a GEPInst with address pointer as a

		PassMgr.addPass(SPIRVLowerConstExprPass());
		PassMgr.addPass(SPIRVRegularizeLLVMPass());

Make storage class of builtins to be Input #2456

Are you sure you want to change the base?

Make storage class of builtins to be Input #2456

Conversation

asudarsa commented Mar 22, 2024 • edited Loading

asudarsa commented Mar 22, 2024

karolherbst commented Mar 22, 2024

asudarsa commented Mar 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asudarsa commented Apr 1, 2024

Choose a reason for hiding this comment

asudarsa commented Apr 4, 2024

Choose a reason for hiding this comment

LU-JOHN Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrSidims Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrSidims Apr 17, 2024 • edited Loading

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

Choose a reason for hiding this comment

MrSidims Apr 17, 2024 • edited Loading

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

asudarsa commented Mar 22, 2024 •

edited

Loading

LU-JOHN Apr 16, 2024 •

edited

Loading

MrSidims Apr 17, 2024 •

edited

Loading

MrSidims Apr 17, 2024 •

edited

Loading

MrSidims Apr 17, 2024 •

edited

Loading