[SPIR-V] Fixup storage class for global private #116636

Keenuts · 2024-11-18T15:07:24Z

Adds a new address spaces: hlsl_private. Variables with such address space will be emitted with a Private storage class.
This is useful for variables global to a SPIR-V module, since up to now, they were still emitted with a Function storage class, which is wrong.

llvmbot · 2024-11-18T15:07:59Z

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-backend-amdgpu
@llvm/pr-subscribers-backend-systemz
@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-backend-spir-v

Author: Nathan Gauër (Keenuts)

Changes

A global private variable was emitted with a Function storage class. This should be Private.

This PR touches 2 other parts:

a test
SPIRVUtils.

The test for SPV_INTEL_function_pointers was testing the storage class of a pointer, and expected "Function". But from what I read, the value should be "CodeSectionINTEL". So since this test was already wrong, updated it to expect "Private", which is as wrong as "Function". Maybe this test should be removed or marked as X-FAIL?

SPIRVUtils: I needed to add AS->SC conversion for Private. Input was already present, but not Output, so added Output just before Private. Input/Output/Private were not used, so picked the next numbers.

Full diff: https://github.com/llvm/llvm-project/pull/116636.diff

6 Files Affected:

(modified) llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp (+13-10)
(modified) llvm/lib/Target/SPIRV/SPIRVUtils.cpp (+4)
(modified) llvm/lib/Target/SPIRV/SPIRVUtils.h (+4)
(modified) llvm/test/CodeGen/SPIRV/extensions/SPV_INTEL_function_pointers/fp_const.ll (+5-1)
(added) llvm/test/CodeGen/SPIRV/pointers/variables-storage-class-vk.ll (+15)
(modified) llvm/test/CodeGen/SPIRV/pointers/variables-storage-class.ll (+18-5)

diff --git a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
index 158314d23393ae..05a4bb12b54cba 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
@@ -3264,9 +3264,15 @@ bool SPIRVInstructionSelector::selectGlobalValue(
     PointerBaseType = GR.getOrCreateSPIRVType(
         GVType, MIRBuilder, SPIRV::AccessQualifier::ReadWrite, false);
   }
-  SPIRVType *ResType = GR.getOrCreateSPIRVPointerType(
-      PointerBaseType, I, TII,
-      addressSpaceToStorageClass(GV->getAddressSpace(), STI));
+
+  const unsigned AddrSpace = GV->getAddressSpace();
+  SPIRV::StorageClass::StorageClass StorageClass =
+      addressSpaceToStorageClass(AddrSpace, STI);
+  if (StorageClass == SPIRV::StorageClass::Function)
+    StorageClass = SPIRV::StorageClass::Private;
+
+  SPIRVType *ResType =
+      GR.getOrCreateSPIRVPointerType(PointerBaseType, I, TII, StorageClass);
 
   std::string GlobalIdent;
   if (!GV->hasName()) {
@@ -3337,11 +3343,8 @@ bool SPIRVInstructionSelector::selectGlobalValue(
   if (HasInit && !Init)
     return true;
 
-  unsigned AddrSpace = GV->getAddressSpace();
-  SPIRV::StorageClass::StorageClass Storage =
-      addressSpaceToStorageClass(AddrSpace, STI);
   bool HasLnkTy = GV->getLinkage() != GlobalValue::InternalLinkage &&
-                  Storage != SPIRV::StorageClass::Function;
+                  StorageClass != SPIRV::StorageClass::Function;
   SPIRV::LinkageType::LinkageType LnkType =
       (GV->isDeclaration() || GV->hasAvailableExternallyLinkage())
           ? SPIRV::LinkageType::Import
@@ -3350,9 +3353,9 @@ bool SPIRVInstructionSelector::selectGlobalValue(
                  ? SPIRV::LinkageType::LinkOnceODR
                  : SPIRV::LinkageType::Export);
 
-  Register Reg = GR.buildGlobalVariable(ResVReg, ResType, GlobalIdent, GV,
-                                        Storage, Init, GlobalVar->isConstant(),
-                                        HasLnkTy, LnkType, MIRBuilder, true);
+  Register Reg = GR.buildGlobalVariable(
+      ResVReg, ResType, GlobalIdent, GV, StorageClass, Init,
+      GlobalVar->isConstant(), HasLnkTy, LnkType, MIRBuilder, true);
   return Reg.isValid();
 }
 
diff --git a/llvm/lib/Target/SPIRV/SPIRVUtils.cpp b/llvm/lib/Target/SPIRV/SPIRVUtils.cpp
index aeb2c29f7b8618..bfa3f50b8f16d9 100644
--- a/llvm/lib/Target/SPIRV/SPIRVUtils.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVUtils.cpp
@@ -205,6 +205,10 @@ addressSpaceToStorageClass(unsigned AddrSpace, const SPIRVSubtarget &STI) {
                : SPIRV::StorageClass::CrossWorkgroup;
   case 7:
     return SPIRV::StorageClass::Input;
+  case 8:
+    return SPIRV::StorageClass::Output;
+  case 9:
+    return SPIRV::StorageClass::Private;
   default:
     report_fatal_error("Unknown address space");
   }
diff --git a/llvm/lib/Target/SPIRV/SPIRVUtils.h b/llvm/lib/Target/SPIRV/SPIRVUtils.h
index 298b0b93b0e4d2..1a3c1399c5a02c 100644
--- a/llvm/lib/Target/SPIRV/SPIRVUtils.h
+++ b/llvm/lib/Target/SPIRV/SPIRVUtils.h
@@ -166,6 +166,10 @@ storageClassToAddressSpace(SPIRV::StorageClass::StorageClass SC) {
     return 6;
   case SPIRV::StorageClass::Input:
     return 7;
+  case SPIRV::StorageClass::Output:
+    return 8;
+  case SPIRV::StorageClass::Private:
+    return 9;
   default:
     report_fatal_error("Unable to get address space id");
   }
diff --git a/llvm/test/CodeGen/SPIRV/extensions/SPV_INTEL_function_pointers/fp_const.ll b/llvm/test/CodeGen/SPIRV/extensions/SPV_INTEL_function_pointers/fp_const.ll
index b4faba9a4eb8e3..a6b5e2fe3c2971 100644
--- a/llvm/test/CodeGen/SPIRV/extensions/SPV_INTEL_function_pointers/fp_const.ll
+++ b/llvm/test/CodeGen/SPIRV/extensions/SPV_INTEL_function_pointers/fp_const.ll
@@ -11,7 +11,11 @@
 ; CHECK-DAG: %[[TyFun:.*]] = OpTypeFunction %[[TyInt64]] %[[TyInt64]]
 ; CHECK-DAG: %[[TyInt8:.*]] = OpTypeInt 8 0
 ; CHECK-DAG: %[[TyPtrFun:.*]] = OpTypePointer Function %[[TyFun]]
-; CHECK-DAG: %[[ConstFunFp:.*]] = OpConstantFunctionPointerINTEL %[[TyPtrFun]] %[[DefFunFp:.*]]
+
+; FIXME: OpConstantFunctionPointerINTEL requires a CodeSectionINTEL ptr.
+; CHECK-DAG: %[[TyPtrPriv:.*]] = OpTypePointer Private %[[TyFun]]
+; CHECK-DAG: %[[ConstFunFp:.*]] = OpConstantFunctionPointerINTEL %[[TyPtrPriv]] %[[DefFunFp:.*]]
+
 ; CHECK-DAG: %[[TyPtrPtrFun:.*]] = OpTypePointer Function %[[TyPtrFun]]
 ; CHECK-DAG: %[[TyPtrInt8:.*]] = OpTypePointer Function %[[TyInt8]]
 ; CHECK-DAG: %[[TyPtrPtrInt8:.*]] = OpTypePointer Function %[[TyPtrInt8]]
diff --git a/llvm/test/CodeGen/SPIRV/pointers/variables-storage-class-vk.ll b/llvm/test/CodeGen/SPIRV/pointers/variables-storage-class-vk.ll
new file mode 100644
index 00000000000000..f7dbef2d01f5fc
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/pointers/variables-storage-class-vk.ll
@@ -0,0 +1,15 @@
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv-unknown-vulkan1.3-compute %s -o - | FileCheck %s
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv-unknown-vulkan1.3-compute %s -o - -filetype=obj | spirv-val %}
+
+; CHECK-DAG: %[[#U32:]] = OpTypeInt 32 0
+
+; CHECK-DAG: %[[#VAL:]] = OpConstant %[[#U32]] 456
+; CHECK-DAG: %[[#VTYPE:]] = OpTypePointer Private %[[#U32]]
+; CHECK-DAG: %[[#VAR:]] = OpVariable %[[#VTYPE]] Private %[[#VAL]]
+; CHECK-NOT: OpDecorate %[[#VAR]] LinkageAttributes
+@PrivInternal = internal global i32 456
+
+define void @main() {
+  %l = load i32, ptr @PrivInternal
+  ret void
+}
diff --git a/llvm/test/CodeGen/SPIRV/pointers/variables-storage-class.ll b/llvm/test/CodeGen/SPIRV/pointers/variables-storage-class.ll
index 2d4c805ac9df15..fdbe2750a0c159 100644
--- a/llvm/test/CodeGen/SPIRV/pointers/variables-storage-class.ll
+++ b/llvm/test/CodeGen/SPIRV/pointers/variables-storage-class.ll
@@ -1,18 +1,31 @@
 ; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s
 ; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
 
+; CHECK-DAG: %[[#U8:]] = OpTypeInt 8 0
+; CHECK-DAG: %[[#U32:]] = OpTypeInt 32 0
+
+; CHECK-DAG: %[[#TYPE:]] = OpTypePointer CrossWorkgroup %[[#U8]]
+; CHECK-DAG: %[[#VAL:]] = OpConstantNull %[[#TYPE]]
+; CHECK-DAG: %[[#VTYPE:]] = OpTypePointer CrossWorkgroup %[[#TYPE]]
+; CHECK-DAG: %[[#PTR:]] = OpVariable %[[#VTYPE]] CrossWorkgroup %[[#VAL]]
 @Ptr = addrspace(1) global ptr addrspace(1) null
-@Init = private addrspace(2) constant i32 123
 
-; CHECK-DAG: %[[#PTR:]] = OpVariable %[[#]] UniformConstant %[[#]]
-; CHECK-DAG: %[[#INIT:]] = OpVariable %[[#]] CrossWorkgroup %[[#]]
+; CHECK-DAG: %[[#VAL:]] = OpConstant %[[#U32]] 123
+; CHECK-DAG: %[[#VTYPE:]] = OpTypePointer UniformConstant %[[#U32]]
+; CHECK-DAG: %[[#INIT:]] = OpVariable %[[#VTYPE]] UniformConstant %[[#VAL]]
+@Init = private addrspace(2) constant i32 123
 
-; CHECK: %[[#]] = OpLoad %[[#]] %[[#INIT]] Aligned 8
-; CHECK: OpCopyMemorySized %[[#]] %[[#PTR]] %[[#]] Aligned 4
+; CHECK-DAG: %[[#VAL:]] = OpConstant %[[#U32]] 456
+; CHECK-DAG: %[[#VTYPE:]] = OpTypePointer Private %[[#U32]]
+; CHECK-DAG: %[[#]] = OpVariable %[[#VTYPE]] Private %[[#VAL]]
+@PrivInternal = internal global i32 456
 
 define spir_kernel void @Foo() {
+; CHECK: %[[#]] = OpLoad %[[#]] %[[#PTR]] Aligned 8
   %l = load ptr addrspace(1), ptr addrspace(1) @Ptr, align 8
+; CHECK: OpCopyMemorySized %[[#]] %[[#INIT]] %[[#]] Aligned 4
   call void @llvm.memcpy.p1.p2.i64(ptr addrspace(1) align 4 %l, ptr addrspace(2) align 1 @Init, i64 4, i1 false)
+
   ret void
 }

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp

llvm/lib/Target/SPIRV/SPIRVUtils.cpp

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp

llvm/test/CodeGen/SPIRV/extensions/SPV_INTEL_function_pointers/fp_const.ll

@MrSidims

…CodeSectionINTEL (#117250) This PR fixes generation of OpConstantFunctionPointerINTEL instruction for the SPIR-V extension SPV_INTEL_function_pointers. Result type of OpConstantFunctionPointerINTEL must be OpTypePointer with Storage Class operand equal to CodeSectionINTEL. See also #116636 CC: @MrSidims

In SPIR-V, private global variables have the `Private` storage class. This PR adds a new address space which allows frontend to emit variable with this storage class. Before this change, global variable were emitted with the 'Function' storage class, which was wrong. Signed-off-by: Nathan Gauër <[email protected]>

Keenuts · 2024-11-28T14:13:43Z

Hello all!
Changed the PR to use a new AS emitted by the FE. This way there are no more weird storage class switch.
However, this required to support a new addrspacecast operation from Function to Private and the other way around.

The FE change will be in another PR (can be merged later on)

Keenuts · 2024-11-28T14:14:13Z

@llvm-beanz : FYI for the added address space.

s-perron

The implementation looks good to me. I just wonder about one of the tests. I'd like to know why we need to handle that case, and how prevalent the addrspacecasts will be.

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp

llvm/test/CodeGen/SPIRV/pointers/global-addrspacecast.ll

Signed-off-by: Nathan Gauër <[email protected]>

MrSidims

LGTM
Guess the PR description should be updated removing CodeSectionINTEL mentioning

arsenm · 2024-12-02T15:39:56Z

clang/include/clang/Basic/AddressSpaces.h


  // HLSL specific address spaces.
  hlsl_groupshared,
+  hlsl_private,


There are no language / source tests making use of this

We hoped to split the PRs between back-end and FE, shall I land both at the same time?

This isn't split though, the backend parts don't require any of these clang changes

Oh didn't knew I could modify the target's AS maps without also modifying this.
I'll revert/re-land this PR (given the other test), and see if I can do that:
#118312

edit: seems like I don't even need to touch the maps until the FE changes emits this new address space. I'll re-land the PR without those bits then.

arsenm · 2024-12-02T15:40:30Z

clang/test/SemaTemplate/address_space-dependent.cpp

  neg<-1>(); // expected-note {{in instantiation of function template specialization 'neg<-1>' requested here}}
  correct<0x7FFFE9>();
-  tooBig<8388650>(); // expected-note {{in instantiation of function template specialization 'tooBig<8388650L>' requested here}}
+  tooBig<8388651>(); // expected-note {{in instantiation of function template specialization 'tooBig<8388651L>' requested here}}


Looks unrelated

This reverts commit aa7fe1c.

…118312) This reverts commit aa7fe1c.

Re-land of llvm#116636 Adds a new address spaces: hlsl_private. Variables with such address space will be emitted with a Private storage class. This is useful for variables global to a SPIR-V module, since up to now, they were still emitted with a Function storage class, which is wrong. Signed-off-by: Nathan Gauër <[email protected]>

Re-land of #116636 Adds a new address spaces: hlsl_private. Variables with such address space will be emitted with a Private storage class. This is useful for variables global to a SPIR-V module, since up to now, they were still emitted with a Function storage class, which is wrong. --------- Signed-off-by: Nathan Gauër <[email protected]>

Keenuts requested a review from VyacheslavLevytskyy November 18, 2024 15:07

llvmbot added the backend:SPIR-V label Nov 18, 2024

Keenuts requested a review from s-perron November 18, 2024 15:07

s-perron reviewed Nov 18, 2024

View reviewed changes

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp Outdated Show resolved Hide resolved

s-perron reviewed Nov 18, 2024

View reviewed changes

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp Outdated Show resolved Hide resolved

MrSidims reviewed Nov 21, 2024

View reviewed changes

llvm/lib/Target/SPIRV/SPIRVUtils.cpp Outdated Show resolved Hide resolved

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp Outdated Show resolved Hide resolved

llvm/test/CodeGen/SPIRV/extensions/SPV_INTEL_function_pointers/fp_const.ll Outdated Show resolved Hide resolved

VyacheslavLevytskyy mentioned this pull request Nov 21, 2024

[SPIR-V] Fix SPIR-V extension SPV_INTEL_function_pointers: introduce CodeSectionINTEL #117250

Merged

Keenuts force-pushed the global-private branch from f831d50 to 30bdda1 Compare November 28, 2024 14:10

llvmbot added clang Clang issues not falling into any other category backend:AArch64 backend:AMDGPU backend:SystemZ backend:WebAssembly clang:frontend Language frontend issues, e.g. anything involving "Sema" backend:DirectX labels Nov 28, 2024

Keenuts requested a review from llvm-beanz November 28, 2024 14:14

Keenuts requested review from MrSidims and s-perron November 29, 2024 12:16

s-perron reviewed Nov 29, 2024

View reviewed changes

llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp Outdated Show resolved Hide resolved

llvm/test/CodeGen/SPIRV/pointers/global-addrspacecast.ll Show resolved Hide resolved

pr feedback

657ae42

Signed-off-by: Nathan Gauër <[email protected]>

MrSidims approved these changes Dec 2, 2024

View reviewed changes

s-perron approved these changes Dec 2, 2024

View reviewed changes

VyacheslavLevytskyy approved these changes Dec 2, 2024

View reviewed changes

Keenuts merged commit aa7fe1c into llvm:main Dec 2, 2024
8 of 9 checks passed

Keenuts deleted the global-private branch December 2, 2024 15:17

arsenm reviewed Dec 2, 2024

View reviewed changes

Keenuts added a commit to Keenuts/llvm-project that referenced this pull request Dec 2, 2024

Revert "[SPIR-V] Fixup storage class for global private (llvm#116636)"

e8c3d6d

This reverts commit aa7fe1c.

Keenuts added a commit that referenced this pull request Dec 2, 2024

Revert "[SPIR-V] Fixup storage class for global private (#116636)" (#…

f8b4182

…118312) This reverts commit aa7fe1c.

Keenuts mentioned this pull request Dec 2, 2024

[SPIR-V] Fixup storage class for global private #118318

Merged

[SPIR-V] Fixup storage class for global private #116636

[SPIR-V] Fixup storage class for global private #116636

Uh oh!

Conversation

Keenuts commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Keenuts commented Nov 28, 2024

Uh oh!

Keenuts commented Nov 28, 2024

Uh oh!

s-perron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MrSidims left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arsenm Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

Keenuts Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

Keenuts Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arsenm Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Keenuts commented Nov 18, 2024 •

edited

Loading

llvmbot commented Nov 18, 2024 •

edited

Loading

MrSidims left a comment •

edited

Loading

Keenuts Dec 2, 2024 •

edited

Loading