[clang][CodeGen] Add AS for Globals to SPIR & SPIRV datalayouts #88455

AlexVlx · 2024-04-11T23:24:25Z

Currently neither the SPIR nor the SPIRV targets specify the AS for globals in their datalayout strings. This is problematic because CodeGen/LLVM will default to AS0 in this case, which produces Globals that end up in the private address space for e.g. OCL, HIPSPV or SYCL. This patch addresses it by completing the datalayout string.

llvmbot · 2024-04-11T23:24:55Z

@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-backend-spir-v

@llvm/pr-subscribers-clang

Author: Alex Voicu (AlexVlx)

Changes

Currently neither the SPIR nor the SPIRV targets specify the AS for globals in their datalayout strings. This is problematic because CodeGen/LLVM will default to AS0 in this case, which produces Globals that end up in the private address space for e.g. OCL, HIPSPV or SYCL. This patch addresses it by completing the datalayout string.

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

2 Files Affected:

(modified) clang/lib/Basic/Targets/SPIR.h (+4-4)
(modified) clang/test/CodeGen/target-data.c (+2-2)

diff --git a/clang/lib/Basic/Targets/SPIR.h b/clang/lib/Basic/Targets/SPIR.h
index e25991e3dfe821..9a4a8b501460b6 100644
--- a/clang/lib/Basic/Targets/SPIR.h
+++ b/clang/lib/Basic/Targets/SPIR.h
@@ -259,7 +259,7 @@ class LLVM_LIBRARY_VISIBILITY SPIR32TargetInfo : public SPIRTargetInfo {
     SizeType = TargetInfo::UnsignedInt;
     PtrDiffType = IntPtrType = TargetInfo::SignedInt;
     resetDataLayout("e-p:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-"
-                    "v96:128-v192:256-v256:256-v512:512-v1024:1024");
+                    "v96:128-v192:256-v256:256-v512:512-v1024:1024-G1");
   }
 
   void getTargetDefines(const LangOptions &Opts,
@@ -276,7 +276,7 @@ class LLVM_LIBRARY_VISIBILITY SPIR64TargetInfo : public SPIRTargetInfo {
     SizeType = TargetInfo::UnsignedLong;
     PtrDiffType = IntPtrType = TargetInfo::SignedLong;
     resetDataLayout("e-i64:64-v16:16-v24:32-v32:32-v48:64-"
-                    "v96:128-v192:256-v256:256-v512:512-v1024:1024");
+                    "v96:128-v192:256-v256:256-v512:512-v1024:1024-G1");
   }
 
   void getTargetDefines(const LangOptions &Opts,
@@ -336,7 +336,7 @@ class LLVM_LIBRARY_VISIBILITY SPIRV32TargetInfo : public BaseSPIRVTargetInfo {
     SizeType = TargetInfo::UnsignedInt;
     PtrDiffType = IntPtrType = TargetInfo::SignedInt;
     resetDataLayout("e-p:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-"
-                    "v96:128-v192:256-v256:256-v512:512-v1024:1024");
+                    "v96:128-v192:256-v256:256-v512:512-v1024:1024-G1");
   }
 
   void getTargetDefines(const LangOptions &Opts,
@@ -357,7 +357,7 @@ class LLVM_LIBRARY_VISIBILITY SPIRV64TargetInfo : public BaseSPIRVTargetInfo {
     SizeType = TargetInfo::UnsignedLong;
     PtrDiffType = IntPtrType = TargetInfo::SignedLong;
     resetDataLayout("e-i64:64-v16:16-v24:32-v32:32-v48:64-"
-                    "v96:128-v192:256-v256:256-v512:512-v1024:1024");
+                    "v96:128-v192:256-v256:256-v512:512-v1024:1024-G1");
   }
 
   void getTargetDefines(const LangOptions &Opts,
diff --git a/clang/test/CodeGen/target-data.c b/clang/test/CodeGen/target-data.c
index acff367d50eb91..c184f314f68f80 100644
--- a/clang/test/CodeGen/target-data.c
+++ b/clang/test/CodeGen/target-data.c
@@ -251,11 +251,11 @@
 
 // RUN: %clang_cc1 -triple spir-unknown -o - -emit-llvm %s | \
 // RUN: FileCheck %s -check-prefix=SPIR
-// SPIR: target datalayout = "e-p:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024"
+// SPIR: target datalayout = "e-p:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-G1"
 
 // RUN: %clang_cc1 -triple spir64-unknown -o - -emit-llvm %s | \
 // RUN: FileCheck %s -check-prefix=SPIR64
-// SPIR64: target datalayout = "e-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024"
+// SPIR64: target datalayout = "e-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-G1"
 
 // RUN: %clang_cc1 -triple bpfel -o - -emit-llvm %s | \
 // RUN: FileCheck %s -check-prefix=BPFEL

bader · 2024-04-12T20:24:00Z

The change seems reasonable.

CodeGen/LLVM will default to AS0 in this case, which produces Globals that end up in the private address space for e.g. OCL, HIPSPV or SYCL.

Can we add a test checking LLVM address space for globals emitted from OCL/HIPSPV/SYCL, please? It's surprising that we need to modify only a datalayout string check.

AlexVlx · 2024-04-12T20:47:36Z

The change seems reasonable.

CodeGen/LLVM will default to AS0 in this case, which produces Globals that end up in the private address space for e.g. OCL, HIPSPV or SYCL.

Can we add a test checking LLVM address space for globals emitted from OCL/HIPSPV/SYCL, please? It's surprising that we need to modify only a datalayout string check.

I can add another one here, but there's a bunch of them coming in #88182, which roundabout motivated this change. I'll emphasise that this is only a problem for things such as implicitly generated globals (e.g. VTables or typeinfo for classes etc.), so it's just a subset of all globals that are impacted (there are already some tests covering direct usage AFAICS).

bader

I'll emphasise that this is only a problem for things such as implicitly generated globals (e.g. VTables or typeinfo for classes etc.)

I suppose usage of VTables and typeinfo is kind of restricted in GPU programming models. Right?

Anyway, I think it's a reasonable change for SPIR target, which fixes mapping from global address space to 1 LLVM address space.
It's less obvious for SPIR-V, which is not LLVM-based format and mapping between SPIR-V and LLVM addresses spaces is technically an implementation detail, which can be adjusted.
According to my understanding, today LLVM IR for SPIR-V target just follows SPIR target nomenclature.

I'd like Eli/John to give formal approval for this change, but I'm okay with it. 👍 Thanks!

asudarsa · 2024-04-15T00:40:30Z

Thanks @AlexVlx for this change. This should work fine for SPIRV-LLVM-Translator (and SPIR-V backend). Adding @michalpaszkowski for input from SPIR-V backend side. Recently, this restriction on LLVM IR input to our translator was docuemnted:
https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/docs/SPIRVRepresentationInLLVM.rst#global-variables
"A global variable resides in an address space, and the default address space in LLVM is zero. The SPIR-V storage class represented by the zero LLVM IR address spaces is Function. However, SPIR-V global variable declarations are OpVariable instructions whose Storage Class cannot be Function. This means that global variable declarations must always have an address space specified and that address space cannot be 0."
So, your change will help to make the LLVM IR more suitable for the translator.

One quick pointer. I did notice a similar commit for the AMDGPU backend - https://reviews.llvm.org/D84345
Here, there are some updates to the llvm/lib/IR/AutoUpgrade.cpp. Do we need similar changes here?

Thanks

AlexVlx · 2024-04-15T00:59:02Z

Thanks @AlexVlx for this change. This should work fine for SPIRV-LLVM-Translator (and SPIR-V backend). Adding @michalpaszkowski for input from SPIR-V backend side. Recently, this restriction on LLVM IR input to our translator was docuemnted: https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/docs/SPIRVRepresentationInLLVM.rst#global-variables "A global variable resides in an address space, and the default address space in LLVM is zero. The SPIR-V storage class represented by the zero LLVM IR address spaces is Function. However, SPIR-V global variable declarations are OpVariable instructions whose Storage Class cannot be Function. This means that global variable declarations must always have an address space specified and that address space cannot be 0." So, your change will help to make the LLVM IR more suitable for the translator.

One quick pointer. I did notice a similar commit for the AMDGPU backend - https://reviews.llvm.org/D84345 Here, there are some updates to the llvm/lib/IR/AutoUpgrade.cpp. Do we need similar changes here?

Thanks

Thanks for the feedback, and great call on the AutoUpgrade part, I had not considered that at all; I believe we can just re-use the AMDGPU approach, and just adapt the predicate, but I'll give it a think and then update this PR accordingly.

michalpaszkowski

LGTM (from the SPIR-V backend side)!

efriedma-quic · 2024-04-15T17:38:06Z

Please also fix llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp

…dl_for_spirv

AlexVlx

Please also fix llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp

Done.

AlexVlx

Thanks @AlexVlx for this change. This should work fine for SPIRV-LLVM-Translator (and SPIR-V backend). Adding @michalpaszkowski for input from SPIR-V backend side. Recently, this restriction on LLVM IR input to our translator was docuemnted: https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/docs/SPIRVRepresentationInLLVM.rst#global-variables "A global variable resides in an address space, and the default address space in LLVM is zero. The SPIR-V storage class represented by the zero LLVM IR address spaces is Function. However, SPIR-V global variable declarations are OpVariable instructions whose Storage Class cannot be Function. This means that global variable declarations must always have an address space specified and that address space cannot be 0." So, your change will help to make the LLVM IR more suitable for the translator.

One quick pointer. I did notice a similar commit for the AMDGPU backend - https://reviews.llvm.org/D84345 Here, there are some updates to the llvm/lib/IR/AutoUpgrade.cpp. Do we need similar changes here?

Thanks

I've added AutoUpgrade support.

asudarsa

Thanks for this change. LGTM

efriedma-quic

LGTM

AlexVlx · 2024-04-16T10:37:47Z

Merged, thanks everyone for the reviews!

bogner · 2024-04-16T17:31:24Z

Looks like this missed a spot, causing the .hlsl to spir-v tests to fail. I've put up a fix in #88939

This was missed in #88455, causing most of the .hlsl to SPIR-V tests to fail (such as clang\test\Driver\hlsl-lang-targets-spirv.hlsl)

Add AS 1 for Globals in the SPIR & SPIRV datalayout strings.

6d9e979

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 11, 2024

AlexVlx added backend:SPIR-V SPIR-V SPIR-V language support clang:codegen and removed clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 11, 2024

AlexVlx requested review from efriedma-quic, jrtc27, yxsamliu, Keenuts and bader April 11, 2024 23:24

bader reviewed Apr 12, 2024

View reviewed changes

AlexVlx mentioned this pull request Apr 14, 2024

[Clang][CodeGen] Start migrating away from assuming the Default AS is 0 #88182

Merged

michalpaszkowski approved these changes Apr 15, 2024

View reviewed changes

Keenuts approved these changes Apr 15, 2024

View reviewed changes

AlexVlx added 2 commits April 15, 2024 19:56

Merge branch 'main' of https://github.com/llvm/llvm-project into fix_…

355e218

…dl_for_spirv

Handle the LLVM side of the SPIRV target.

cd2d855

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 15, 2024

Augment AutoUpgrade infra to handle legacy SPIR & SPIRV datalayouts.

4becde6

AlexVlx commented Apr 15, 2024

View reviewed changes

llvmbot added the llvm:ir label Apr 15, 2024

AlexVlx commented Apr 15, 2024

View reviewed changes

asudarsa approved these changes Apr 15, 2024

View reviewed changes

efriedma-quic approved these changes Apr 15, 2024

View reviewed changes

AlexVlx merged commit 1120d8e into llvm:main Apr 16, 2024
6 checks passed

AlexVlx deleted the fix_dl_for_spirv branch April 16, 2024 10:39

AlexVlx added a commit to AlexVlx/llvm-project that referenced this pull request Apr 16, 2024

Roll-back Global generation to use defaults now that llvm#88455 went in.

c18febe

Keenuts mentioned this pull request Apr 16, 2024

[SPIR-V] error: backend data layout does not match expected target description #88936

Closed

bogner mentioned this pull request Apr 16, 2024

[clang][SPIR-V] Set AS for the SPIR-V logical triple #88939

Merged

bogner added a commit that referenced this pull request Apr 16, 2024

[clang][SPIR-V] Set AS for the SPIR-V logical triple (#88939)

b0ddbfb

This was missed in #88455, causing most of the .hlsl to SPIR-V tests to fail (such as clang\test\Driver\hlsl-lang-targets-spirv.hlsl)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang][CodeGen] Add AS for Globals to SPIR & SPIRV datalayouts #88455

[clang][CodeGen] Add AS for Globals to SPIR & SPIRV datalayouts #88455

AlexVlx commented Apr 11, 2024

llvmbot commented Apr 11, 2024 •

edited

Loading

bader commented Apr 12, 2024

AlexVlx commented Apr 12, 2024

bader left a comment

asudarsa commented Apr 15, 2024

AlexVlx commented Apr 15, 2024

michalpaszkowski left a comment

efriedma-quic commented Apr 15, 2024

AlexVlx left a comment

AlexVlx left a comment

asudarsa left a comment

efriedma-quic left a comment

AlexVlx commented Apr 16, 2024

bogner commented Apr 16, 2024

[clang][CodeGen] Add AS for Globals to SPIR & SPIRV datalayouts #88455

[clang][CodeGen] Add AS for Globals to SPIR & SPIRV datalayouts #88455

Conversation

AlexVlx commented Apr 11, 2024

llvmbot commented Apr 11, 2024 • edited Loading

bader commented Apr 12, 2024

AlexVlx commented Apr 12, 2024

bader left a comment

Choose a reason for hiding this comment

asudarsa commented Apr 15, 2024

AlexVlx commented Apr 15, 2024

michalpaszkowski left a comment

Choose a reason for hiding this comment

efriedma-quic commented Apr 15, 2024

AlexVlx left a comment

Choose a reason for hiding this comment

AlexVlx left a comment

Choose a reason for hiding this comment

asudarsa left a comment

Choose a reason for hiding this comment

efriedma-quic left a comment

Choose a reason for hiding this comment

AlexVlx commented Apr 16, 2024

bogner commented Apr 16, 2024

llvmbot commented Apr 11, 2024 •

edited

Loading