[NFC][AMDGPU] Move address space enum to LLVM directory #73944

DominikAdamski · 2023-11-30T14:30:40Z

Types of AMDGPU address space were defined in Clang-specific class. In consequence this enumeration cannot be reused by other frontends like Flang.

If we move AMDGPU address space enum to LLVM directory, then we can reuse it in other frontends like Flang.

Types of AMDGPU address space were defined in Clang-specific class. In consequence this enum cannot be reused by other frontends like Flang. If we move address space enum to LLVM directory, then we can reuse it in other frontends like Flang.

llvmbot · 2023-11-30T14:31:14Z

@llvm/pr-subscribers-llvm-support
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-flang-driver

Author: Dominik Adamski (DominikAdamski)

Changes

Types of AMDGPU address space were defined in Clang-specific class. In consequence this enumeration cannot be reused by other frontends like Flang.

If we move AMDGPU address space enum to LLVM directory, then we can reuse it in other frontends like Flang.

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

4 Files Affected:

(modified) clang/lib/Basic/Targets/AMDGPU.cpp (+40-40)
(modified) clang/lib/Basic/Targets/AMDGPU.h (+5-12)
(modified) flang/lib/Frontend/FrontendActions.cpp (+2-8)
(modified) llvm/include/llvm/TargetParser/TargetParser.h (+9)

diff --git a/clang/lib/Basic/Targets/AMDGPU.cpp b/clang/lib/Basic/Targets/AMDGPU.cpp
index 409ae32ab424215..3fe9f9fa9c42dea 100644
--- a/clang/lib/Basic/Targets/AMDGPU.cpp
+++ b/clang/lib/Basic/Targets/AMDGPU.cpp
@@ -37,50 +37,50 @@ static const char *const DataLayoutStringAMDGCN =
     "-ni:7:8";
 
 const LangASMap AMDGPUTargetInfo::AMDGPUDefIsGenMap = {
-    Generic,  // Default
-    Global,   // opencl_global
-    Local,    // opencl_local
-    Constant, // opencl_constant
-    Private,  // opencl_private
-    Generic,  // opencl_generic
-    Global,   // opencl_global_device
-    Global,   // opencl_global_host
-    Global,   // cuda_device
-    Constant, // cuda_constant
-    Local,    // cuda_shared
-    Global,   // sycl_global
-    Global,   // sycl_global_device
-    Global,   // sycl_global_host
-    Local,    // sycl_local
-    Private,  // sycl_private
-    Generic,  // ptr32_sptr
-    Generic,  // ptr32_uptr
-    Generic,  // ptr64
-    Generic,  // hlsl_groupshared
+    llvm::AMDGPU::Generic,  // Default
+    llvm::AMDGPU::Global,   // opencl_global
+    llvm::AMDGPU::Local,    // opencl_local
+    llvm::AMDGPU::Constant, // opencl_constant
+    llvm::AMDGPU::Private,  // opencl_private
+    llvm::AMDGPU::Generic,  // opencl_generic
+    llvm::AMDGPU::Global,   // opencl_global_device
+    llvm::AMDGPU::Global,   // opencl_global_host
+    llvm::AMDGPU::Global,   // cuda_device
+    llvm::AMDGPU::Constant, // cuda_constant
+    llvm::AMDGPU::Local,    // cuda_shared
+    llvm::AMDGPU::Global,   // sycl_global
+    llvm::AMDGPU::Global,   // sycl_global_device
+    llvm::AMDGPU::Global,   // sycl_global_host
+    llvm::AMDGPU::Local,    // sycl_local
+    llvm::AMDGPU::Private,  // sycl_private
+    llvm::AMDGPU::Generic,  // ptr32_sptr
+    llvm::AMDGPU::Generic,  // ptr32_uptr
+    llvm::AMDGPU::Generic,  // ptr64
+    llvm::AMDGPU::Generic,  // hlsl_groupshared
 };
 
 const LangASMap AMDGPUTargetInfo::AMDGPUDefIsPrivMap = {
-    Private,  // Default
-    Global,   // opencl_global
-    Local,    // opencl_local
-    Constant, // opencl_constant
-    Private,  // opencl_private
-    Generic,  // opencl_generic
-    Global,   // opencl_global_device
-    Global,   // opencl_global_host
-    Global,   // cuda_device
-    Constant, // cuda_constant
-    Local,    // cuda_shared
+    llvm::AMDGPU::Private,  // Default
+    llvm::AMDGPU::Global,   // opencl_global
+    llvm::AMDGPU::Local,    // opencl_local
+    llvm::AMDGPU::Constant, // opencl_constant
+    llvm::AMDGPU::Private,  // opencl_private
+    llvm::AMDGPU::Generic,  // opencl_generic
+    llvm::AMDGPU::Global,   // opencl_global_device
+    llvm::AMDGPU::Global,   // opencl_global_host
+    llvm::AMDGPU::Global,   // cuda_device
+    llvm::AMDGPU::Constant, // cuda_constant
+    llvm::AMDGPU::Local,    // cuda_shared
     // SYCL address space values for this map are dummy
-    Generic, // sycl_global
-    Generic, // sycl_global_device
-    Generic, // sycl_global_host
-    Generic, // sycl_local
-    Generic, // sycl_private
-    Generic, // ptr32_sptr
-    Generic, // ptr32_uptr
-    Generic, // ptr64
-    Generic, // hlsl_groupshared
+    llvm::AMDGPU::Generic, // sycl_global
+    llvm::AMDGPU::Generic, // sycl_global_device
+    llvm::AMDGPU::Generic, // sycl_global_host
+    llvm::AMDGPU::Generic, // sycl_local
+    llvm::AMDGPU::Generic, // sycl_private
+    llvm::AMDGPU::Generic, // ptr32_sptr
+    llvm::AMDGPU::Generic, // ptr32_uptr
+    llvm::AMDGPU::Generic, // ptr64
+    llvm::AMDGPU::Generic, // hlsl_groupshared
 
 };
 } // namespace targets
diff --git a/clang/lib/Basic/Targets/AMDGPU.h b/clang/lib/Basic/Targets/AMDGPU.h
index 300d9691d8a0f22..1e12f9e12af594c 100644
--- a/clang/lib/Basic/Targets/AMDGPU.h
+++ b/clang/lib/Basic/Targets/AMDGPU.h
@@ -29,13 +29,6 @@ class LLVM_LIBRARY_VISIBILITY AMDGPUTargetInfo final : public TargetInfo {
 
   static const char *const GCCRegNames[];
 
-  enum AddrSpace {
-    Generic = 0,
-    Global = 1,
-    Local = 3,
-    Constant = 4,
-    Private = 5
-  };
   static const LangASMap AMDGPUDefIsGenMap;
   static const LangASMap AMDGPUDefIsPrivMap;
 
@@ -106,7 +99,7 @@ class LLVM_LIBRARY_VISIBILITY AMDGPUTargetInfo final : public TargetInfo {
       return 32;
     unsigned TargetAS = getTargetAddressSpace(AS);
 
-    if (TargetAS == Private || TargetAS == Local)
+    if (TargetAS == llvm::AMDGPU::Private || TargetAS == llvm::AMDGPU::Local)
       return 32;
 
     return 64;
@@ -376,7 +369,7 @@ class LLVM_LIBRARY_VISIBILITY AMDGPUTargetInfo final : public TargetInfo {
   }
 
   std::optional<LangAS> getConstantAddressSpace() const override {
-    return getLangASFromTargetAS(Constant);
+    return getLangASFromTargetAS(llvm::AMDGPU::Constant);
   }
 
   const llvm::omp::GV &getGridValue() const override {
@@ -392,7 +385,7 @@ class LLVM_LIBRARY_VISIBILITY AMDGPUTargetInfo final : public TargetInfo {
 
   /// \returns Target specific vtbl ptr address space.
   unsigned getVtblPtrAddressSpace() const override {
-    return static_cast<unsigned>(Constant);
+    return static_cast<unsigned>(llvm::AMDGPU::Constant);
   }
 
   /// \returns If a target requires an address within a target specific address
@@ -405,9 +398,9 @@ class LLVM_LIBRARY_VISIBILITY AMDGPUTargetInfo final : public TargetInfo {
   getDWARFAddressSpace(unsigned AddressSpace) const override {
     const unsigned DWARF_Private = 1;
     const unsigned DWARF_Local = 2;
-    if (AddressSpace == Private) {
+    if (AddressSpace == llvm::AMDGPU::Private) {
       return DWARF_Private;
-    } else if (AddressSpace == Local) {
+    } else if (AddressSpace == llvm::AMDGPU::Local) {
       return DWARF_Local;
     } else {
       return std::nullopt;
diff --git a/flang/lib/Frontend/FrontendActions.cpp b/flang/lib/Frontend/FrontendActions.cpp
index 1be95cc27f42cd3..3741668eea67d43 100644
--- a/flang/lib/Frontend/FrontendActions.cpp
+++ b/flang/lib/Frontend/FrontendActions.cpp
@@ -272,12 +272,6 @@ static void addAMDGPUSpecificMLIRItems(mlir::ModuleOp &mlirModule,
   const llvm::Triple triple(targetOpts.triple);
   const llvm::StringRef codeObjectVersionGlobalOpName = "__oclc_ABI_version";
 
-  // TODO: Share address spaces enumeration between Clang and Flang.
-  // Currently this enumeration is defined in Clang specific class
-  // defined in file: clang/lib/Basic/Targets/AMDGPU.h .
-  // and we need to move it to LLVM directory.
-  const int constantAddressSpace = 4;
-
   if (!triple.isAMDGPU()) {
     return;
   }
@@ -308,7 +302,7 @@ static void addAMDGPUSpecificMLIRItems(mlir::ModuleOp &mlirModule,
     originalGVOp.setValueAttr(
         builder.getIntegerAttr(int32Type, oclcABIVERsion));
     originalGVOp.setUnnamedAddr(mlir::LLVM::UnnamedAddr::Local);
-    originalGVOp.setAddrSpace(constantAddressSpace);
+    originalGVOp.setAddrSpace(llvm::AMDGPU::Constant);
     originalGVOp.setVisibility_(mlir::LLVM::Visibility::Hidden);
     return;
   }
@@ -319,7 +313,7 @@ static void addAMDGPUSpecificMLIRItems(mlir::ModuleOp &mlirModule,
       /* Name */ codeObjectVersionGlobalOpName,
       /* Value */ builder.getIntegerAttr(int32Type, oclcABIVERsion));
   covInfo.setUnnamedAddr(mlir::LLVM::UnnamedAddr::Local);
-  covInfo.setAddrSpace(constantAddressSpace);
+  covInfo.setAddrSpace(llvm::AMDGPU::Constant);
   covInfo.setVisibility_(mlir::LLVM::Visibility::Hidden);
   builder.setInsertionPointToStart(mlirModule.getBody());
   builder.insert(covInfo);
diff --git a/llvm/include/llvm/TargetParser/TargetParser.h b/llvm/include/llvm/TargetParser/TargetParser.h
index 6464285980f00a1..b0df831c475675a 100644
--- a/llvm/include/llvm/TargetParser/TargetParser.h
+++ b/llvm/include/llvm/TargetParser/TargetParser.h
@@ -31,6 +31,15 @@ class Triple;
 // back-end to TableGen to create these clean tables.
 namespace AMDGPU {
 
+/// Address space values for AMD GPUs
+enum AddrSpace {
+  Generic = 0,
+  Global = 1,
+  Local = 3,
+  Constant = 4,
+  Private = 5
+};
+
 /// GPU kinds supported by the AMDGPU target.
 enum GPUKind : uint32_t {
   // Not specified processor.

skatrak

Thanks for this work. I only have very little feedback, but it LGTM.

I think the enum looks a bit out of place in a file called "TargetParser.h", but I'm not very familiar with this part of the project, so I'll let the experts say something if there is a more suitable location for it.

skatrak · 2023-11-30T15:04:47Z

llvm/include/llvm/TargetParser/TargetParser.h

@@ -31,6 +31,15 @@ class Triple;
 // back-end to TableGen to create these clean tables.
 namespace AMDGPU {

+/// Address space values for AMD GPUs
+enum AddrSpace {
+  Generic = 0,


I think it would be good to prefix these names with something so that uses are self-documenting. For example, something like AS_{GENERIC, GLOBAL, ...} or ADDRESS_{GENERIC, GLOBAL, ...}, if we use the enums below as reference.

We could change this enum to enum class.

Done. Moved to enum class.

skatrak · 2023-11-30T15:07:35Z

clang/lib/Basic/Targets/AMDGPU.h

@@ -405,9 +398,9 @@ class LLVM_LIBRARY_VISIBILITY AMDGPUTargetInfo final : public TargetInfo {
  getDWARFAddressSpace(unsigned AddressSpace) const override {
    const unsigned DWARF_Private = 1;
    const unsigned DWARF_Local = 2;
-    if (AddressSpace == Private) {
+    if (AddressSpace == llvm::AMDGPU::Private) {


Nit: switch?

kparzysz · 2023-11-30T17:18:52Z

I think the enum looks a bit out of place in a file called "TargetParser.h", but I'm not very familiar with this part of the project, so I'll let the experts say something if there is a more suitable location for it.

That's a good point. The address spaces for AMDGPU are already defined here. I think it makes sense to extract them into a separate file (including the static functions to query address spaces), and put that file in llvm/include/llvm/Support. We already have two AMDGPU-specific files in there, so there is a precedent that we can follow.

skatrak

LGTM, thanks! Please wait for one more approval before merging.

DominikAdamski · 2023-12-05T13:19:01Z

The address spaces for AMDGPU defined here contain more types of address spaces in comparison to the enum defined in clang. Is it ok to extend number of address space types for clang?

kparzysz · 2023-12-05T13:22:26Z

The address spaces for AMDGPU defined here contain more types of address spaces in comparison to the enum defined in clang. Is it ok to extend number of address space types for clang?

I'd just move these definitions to their own file, and use that file in clang/flang/llvm backend. So, indirectly, yes.

lenary · 2023-12-05T13:29:43Z

The reason TargetParser exists is because Target directories are only added to the include path if you enable that target, but some cross-project target code needs to always be available, such as the code for Triples, even when the relevant targets are disabled. I think these address space constants have the same needs, so this location makes sense to me, but I could be wrong.

It is strange that the AMDGPU stuff is in the file called TargetParser.h, but that's because I wasn't able to move that code around when I originally split out TargetParser. Now might be a good time to put it in its own header, like was done for all other targets.

kparzysz · 2023-12-05T13:34:53Z

I'd just move these definitions to their own file, and use that file in clang/flang/llvm backend. So, indirectly, yes.

Sorry, didn't notice you have already created a separate file: 👍 I'd just use the address space identifiers that you quoted instead.

kparzysz · 2023-12-05T13:37:12Z

llvm/include/llvm/Support/AMDGPUAddrSpace.h

+
+namespace llvm {
+namespace AMDGPU {
+enum class AddrSpace {


You can explicitly say enum class AddrSpace : unsigned { ... to avoid the need for the static_casts to unsigned.

So this is still a second copy of the address space enum, another copy of which exists in AMDGPU.h (which does not use enum class, and uses different names). What's the plan to consolidate these?

@arsenm I can consolidate these enums but I would like to be sure that I am allowed to do it. I was not sure why Clang enum has smaller range in comparison to LLVM enum.
My initial aim of this patch was to perform minimal code refactoring so that Clang and Flang can reuse the same enum. If you wish I can consolidate Flang, Clang and LLVM enums.

The clang enum is very OpenCL centric and ignores AMDGPU and graphics specific address spaces. I don't see a point in keeping two overlapping enums

@arsenm done. I unified the clang enum with LLVM enum. I haven't modified the names of LLVM enum and I haven't introduced enum class instead of AMDGPUAS namespace. Please let me know if is ok for you.

DominikAdamski · 2023-12-06T13:29:40Z

@lenary Thank you for your input.
@kparzysz @lenary Shall I add all address spaces which are mentioned in other LLVM header? Currently I added address spaces which were mentioned in Clang .

This reverts commit 0d2ea7f.

github-actions · 2023-12-08T09:57:20Z

✅ With the latest revision this PR passed the C/C++ code formatter.

kparzysz

LGTM, thanks!

DominikAdamski requested review from lenary, bader, skatrak, jsjodin, agozillon, dpalermo, python3kgae, TIFitis, kiranktp and kparzysz November 30, 2023 14:30

llvmbot added clang Clang issues not falling into any other category backend:AMDGPU clang:frontend Language frontend issues, e.g. anything involving "Sema" flang:driver flang Flang issues not falling into any other category labels Nov 30, 2023

skatrak reviewed Nov 30, 2023

View reviewed changes

Applied review remarks

0d2ea7f

llvmbot added the llvm:support label Dec 5, 2023

skatrak approved these changes Dec 5, 2023

View reviewed changes

kparzysz reviewed Dec 5, 2023

View reviewed changes

Revert "Applied review remarks"

7be8ec1

This reverts commit 0d2ea7f.

uniform address spaces

09b7fcf

DominikAdamski force-pushed the refactor_address_space branch from 82ec7a5 to 09b7fcf Compare December 8, 2023 10:40

kparzysz approved these changes Dec 8, 2023

View reviewed changes

DominikAdamski merged commit 276a024 into llvm:main Dec 11, 2023
4 checks passed

DominikAdamski deleted the refactor_address_space branch December 11, 2023 09:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NFC][AMDGPU] Move address space enum to LLVM directory #73944

[NFC][AMDGPU] Move address space enum to LLVM directory #73944

DominikAdamski commented Nov 30, 2023

llvmbot commented Nov 30, 2023 •

edited

skatrak left a comment

skatrak Nov 30, 2023

kparzysz Nov 30, 2023

DominikAdamski Dec 5, 2023

skatrak Nov 30, 2023

kparzysz commented Nov 30, 2023

skatrak left a comment

DominikAdamski commented Dec 5, 2023

kparzysz commented Dec 5, 2023

lenary commented Dec 5, 2023

kparzysz commented Dec 5, 2023

kparzysz Dec 5, 2023

arsenm Dec 6, 2023

DominikAdamski Dec 7, 2023

arsenm Dec 7, 2023

DominikAdamski Dec 8, 2023

DominikAdamski commented Dec 6, 2023

github-actions bot commented Dec 8, 2023 •

edited

kparzysz left a comment

[NFC][AMDGPU] Move address space enum to LLVM directory #73944

[NFC][AMDGPU] Move address space enum to LLVM directory #73944

Conversation

DominikAdamski commented Nov 30, 2023

llvmbot commented Nov 30, 2023 • edited

skatrak left a 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

kparzysz commented Nov 30, 2023

skatrak left a comment

Choose a reason for hiding this comment

DominikAdamski commented Dec 5, 2023

kparzysz commented Dec 5, 2023

lenary commented Dec 5, 2023

kparzysz commented Dec 5, 2023

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

DominikAdamski commented Dec 6, 2023

github-actions bot commented Dec 8, 2023 • edited

kparzysz left a comment

Choose a reason for hiding this comment

llvmbot commented Nov 30, 2023 •

edited

github-actions bot commented Dec 8, 2023 •

edited