[AMDGPU] Switch address space mapping by triple environment

[yaxunl]

Summary

For target environment amdgiz and amdgizcl (giz means Generic Is Zero), AMDGPU will use new address space mapping where generic address space is 0 and private address space is 5. The data layout is also changed correspondingly.

This is the beginning of an upstreaming effort for changing address space mapping of AMDGPU target. Sema/CodeGen changes to make OpenCL/C++ working with the new address space mapping will follow.

The depending LLVM change: https://reviews.llvm.org/D31211

[kzhuravl]

getDWARFAddressSpace also needs to be updated.

[rampitec]

I'm concerned about the default address space to be 64 bit. It would move alloca into generic address space effectively making private address to be 64 bit.
This may have very undesirable performance implications, like address arithmetic can become expensive 64 bit and only be truncated at load or store.
I realize you will use addrspacecast on an alloca's value, though I'm not sure that is sufficient to mitigate performance hit.
I believe such change shall not be made without a good performance comparison with the feature enabled, provided the very likely performance issues.

[kzhuravl]

In D31210#706880, @rampitec wrote:

I'm concerned about the default address space to be 64 bit. It would move alloca into generic address space effectively making private address to be 64 bit.
This may have very undesirable performance implications, like address arithmetic can become expensive 64 bit and only be truncated at load or store.
I realize you will use addrspacecast on an alloca's value, though I'm not sure that is sufficient to mitigate performance hit.
I believe such change shall not be made without a good performance comparison with the feature enabled, provided the very likely performance issues.

Did not we want to use this: http://lists.llvm.org/pipermail/llvm-dev/2017-March/111199.html and use non-0 for our allocas?

[yaxunl]

In D31210#706890, @kzhuravl wrote:

In D31210#706880, @rampitec wrote:

I'm concerned about the default address space to be 64 bit. It would move alloca into generic address space effectively making private address to be 64 bit.
This may have very undesirable performance implications, like address arithmetic can become expensive 64 bit and only be truncated at load or store.
I realize you will use addrspacecast on an alloca's value, though I'm not sure that is sufficient to mitigate performance hit.
I believe such change shall not be made without a good performance comparison with the feature enabled, provided the very likely performance issues.

Did not we want to use this: http://lists.llvm.org/pipermail/llvm-dev/2017-March/111199.html and use non-0 for our allocas?

Our final goal is to let alloca return private pointer. The Clang changes are mostly common whether alloca returns generic pointer or private pointer. Actually to be able to test the above patch we need to get the changes in Clang done first.

[scchan]

In D31210#706955, @yaxunl wrote:

In D31210#706890, @kzhuravl wrote:

In D31210#706880, @rampitec wrote:

I'm concerned about the default address space to be 64 bit. It would move alloca into generic address space effectively making private address to be 64 bit.
This may have very undesirable performance implications, like address arithmetic can become expensive 64 bit and only be truncated at load or store.
I realize you will use addrspacecast on an alloca's value, though I'm not sure that is sufficient to mitigate performance hit.
I believe such change shall not be made without a good performance comparison with the feature enabled, provided the very likely performance issues.

Did not we want to use this: http://lists.llvm.org/pipermail/llvm-dev/2017-March/111199.html and use non-0 for our allocas?

Our final goal is to let alloca return private pointer. The Clang changes are mostly common whether alloca returns generic pointer or private pointer. Actually to be able to test the above patch we need to get the changes in Clang done first.

The goal for this is mainly to bring the mapping closer to nvptx?

[yaxunl]

In D31210#707014, @scchan wrote:

In D31210#706955, @yaxunl wrote:

In D31210#706890, @kzhuravl wrote:

In D31210#706880, @rampitec wrote:

I'm concerned about the default address space to be 64 bit. It would move alloca into generic address space effectively making private address to be 64 bit.
This may have very undesirable performance implications, like address arithmetic can become expensive 64 bit and only be truncated at load or store.
I realize you will use addrspacecast on an alloca's value, though I'm not sure that is sufficient to mitigate performance hit.
I believe such change shall not be made without a good performance comparison with the feature enabled, provided the very likely performance issues.

Did not we want to use this: http://lists.llvm.org/pipermail/llvm-dev/2017-March/111199.html and use non-0 for our allocas?

Our final goal is to let alloca return private pointer. The Clang changes are mostly common whether alloca returns generic pointer or private pointer. Actually to be able to test the above patch we need to get the changes in Clang done first.

The goal for this is mainly to bring the mapping closer to nvptx?

Our goal is to be able to support both OpenCL and C++-based kernel languages without degrading OpenCL's performance. To achieve that goal alloca returning private pointer may be necessary.

[yaxunl]

Fix getDWARFAddressSpace.

[tstellar]

lib/Basic/Targets.cpp

2029–2040 ↗

(On Diff #92587)

What are these values used for?

2057 ↗

(On Diff #92587)

How will the backend deal with the fact that allocas return generic address space pointers?

[rampitec]

I also do not exactly like names "old" and "new". This implies we are going to switch to "new" permanently and doing transition. That is not clear yet, however.

[yaxunl]

lib/Basic/Targets.cpp

2029–2040 ↗

(On Diff #92587)

The new address space mapping will be used for all kernel languages, especially for C++-based kernel languages like HCC/OpenMP/CUDA.

We found the old address space mapping is unable to support C++ based kernel languages, that's the motivation of this change.

Eventually we will switch to the new address space mapping.

We temporarily keep two address space mapping during the transition,

2057 ↗

(On Diff #92587)

Matt will upstream a patch which let alloca return a pointer to private address space.

[yaxunl]

In D31210#707832, @rampitec wrote:

I also do not exactly like names "old" and "new". This implies we are going to switch to "new" permanently and doing transition. That is not clear yet, however.

How about we call the old address space mapping "Alloca Is Zero" (AIZ) address space mapping, whereas the new address space mapping "Generic Is Zero" (GIZ) address space mapping?

I can change the variable names. Would you suggest to change the target triple environment name too? e.g. amdnas => amdgiz, amdnascl => amdgizcl?

[rampitec]

In D31210#707842, @yaxunl wrote:

In D31210#707832, @rampitec wrote:

I also do not exactly like names "old" and "new". This implies we are going to switch to "new" permanently and doing transition. That is not clear yet, however.

How about we call the old address space mapping "Alloca Is Zero" (AIZ) address space mapping, whereas the new address space mapping "Generic Is Zero" (GIZ) address space mapping?

I can change the variable names. Would you suggest to change the target triple environment name too? e.g. amdnas => amdgiz, amdnascl => amdgizcl?

I think "private is zero" is better than "alloca is zero". The latter may change. I also think it make sense for these to match evn name. What "nas" suffix stands for by the way?

[yaxunl]

In D31210#707842, @yaxunl wrote:

In D31210#707832, @rampitec wrote:

I also do not exactly like names "old" and "new". This implies we are going to switch to "new" permanently and doing transition. That is not clear yet, however.

How about we call the old address space mapping "Alloca Is Zero" (AIZ) address space mapping, whereas the new address space mapping "Generic Is Zero" (GIZ) address space mapping?

I can change the variable names. Would you suggest to change the target triple environment name too? e.g. amdnas => amdgiz, amdnascl => amdgizcl?

I understand your indication. If we found the maintenance

In D31210#707855, @rampitec wrote:

In D31210#707842, @yaxunl wrote:

In D31210#707832, @rampitec wrote:

I also do not exactly like names "old" and "new". This implies we are going to switch to "new" permanently and doing transition. That is not clear yet, however.

How about we call the old address space mapping "Alloca Is Zero" (AIZ) address space mapping, whereas the new address space mapping "Generic Is Zero" (GIZ) address space mapping?

I can change the variable names. Would you suggest to change the target triple environment name too? e.g. amdnas => amdgiz, amdnascl => amdgizcl?

I think "private is zero" is better than "alloca is zero". The latter may change. I also think it make sense for these to match evn name. What "nas" suffix stands for by the way?

nas means "New Address Space".

[yaxunl]

Rename the triple and variable names.

[yaxunl]

Ping! Any further issues? We need this in to move on with Clang codegen for the new address space mapping.

Thanks.

[yaxunl]

Revised by Tony's suggestions.

[t-tye]

Also please upload as full diff.

lib/Basic/Targets.cpp

2026–2069 ↗

(On Diff #92717)

Now that the target triple is being used to control the address space mapping, the mapping is only being set once. So suggest simplifying this and simply have two static arrays for the address spaces (like is done for the data layout), and then set AddrSpaceMap to the address of the appropriate array.

2119 ↗

(On Diff #92717)

See comment above.

[yaxunl]

Hi Tony,

I have already updated with a full diff. Please take a look. Thanks.

[yaxunl]

Use triple environment name.

[t-tye]

Just a couple of suggestions, otherwise:

LGTM

lib/Basic/Targets.cpp

2015 ↗

(On Diff #92997)

Is there a reason this is no longer const?

2024 ↗

(On Diff #92997)

Same as above.

2357 ↗

(On Diff #92997)

Could this be const since it is not changed after construction?

[yaxunl]

lib/Basic/Targets.cpp

2015 ↗

(On Diff #92997)

sorry, my omission. Will fix it when commit.

2357 ↗

(On Diff #92997)

Yes. will do.

cfe/trunk/lib/Basic/Targets.cpp

 };
 
 const char *const NVPTXTargetInfo::GCCRegNames[] = {"r0"};
 
 ArrayRef<const char *> NVPTXTargetInfo::getGCCRegNames() const {
   return llvm::makeArrayRef(GCCRegNames);
 }
 
-static const unsigned AMDGPUAddrSpaceMap[] = {
-  1,    // opencl_global
-  3,    // opencl_local
-  2,    // opencl_constant
-  4,    // opencl_generic
-  1,    // cuda_device
-  2,    // cuda_constant
-  3     // cuda_shared
+static const LangAS::Map AMDGPUPrivateIsZeroMap = {
+    1,  // opencl_global
+    3,  // opencl_local
+    2,  // opencl_constant
+    4,  // opencl_generic
+    1,  // cuda_device
+    2,  // cuda_constant
+    3   // cuda_shared
 };
+static const LangAS::Map AMDGPUGenericIsZeroMap = {
+    1,  // opencl_global
+    3,  // opencl_local
+    4,  // opencl_constant
+    0,  // opencl_generic
+    1,  // cuda_device
+    4,  // cuda_constant
+    3   // cuda_shared
+};
 
 // If you edit the description strings, make sure you update
 // getPointerWidthV().
 
 static const char *const DataLayoutStringR600 =
   "e-p:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128"
   "-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64";
 
-static const char *const DataLayoutStringSI =
+static const char *const DataLayoutStringSIPrivateIsZero =
   "e-p:32:32-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5:32:32"
   "-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128"
   "-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64";
 
+static const char *const DataLayoutStringSIGenericIsZero =
+  "e-p:64:64-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5:32:32"
+  "-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128"
+  "-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64";
+
 class AMDGPUTargetInfo final : public TargetInfo {
   static const Builtin::Info BuiltinInfo[];
   static const char * const GCCRegNames[];
 
+  struct AddrSpace {
+    unsigned Generic, Global, Local, Constant, Private;
+    AddrSpace(bool IsGenericZero_ = false){
+      if (IsGenericZero_) {
+        Generic   = 0;
+        Global    = 1;
+        Local     = 3;
+        Constant  = 4;
+        Private   = 5;
+      } else {
+        Generic   = 4;
+        Global    = 1;
+        Local     = 3;
+        Constant  = 2;
+        Private   = 0;
+      }
+    }
+  };
+
   /// \brief The GPU profiles supported by the AMDGPU target.
   enum GPUKind {
     GK_NONE,
     GK_R600,
     GK_R600_DOUBLE_OPS,
     GK_R700,
     GK_R700_DOUBLE_OPS,
     GK_EVERGREEN,
class AMDGPUTargetInfo final : public TargetInfo {
   bool hasFMAF:1;
   bool hasLDEXPF:1;
   bool hasFullSpeedFP32Denorms:1;
 
   static bool isAMDGCN(const llvm::Triple &TT) {
     return TT.getArch() == llvm::Triple::amdgcn;
   }
 
+  static bool isGenericZero(const llvm::Triple &TT) {
+    return TT.getEnvironmentName() == "amdgiz" ||
+        TT.getEnvironmentName() == "amdgizcl";
+  }
 public:
   AMDGPUTargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
     : TargetInfo(Triple) ,
       GPU(isAMDGCN(Triple) ? GK_GFX6 : GK_R600),
       hasFP64(false),
       hasFMAF(false),
       hasLDEXPF(false),
-      hasFullSpeedFP32Denorms(false){
+      hasFullSpeedFP32Denorms(false),
+      AS(isGenericZero(Triple)){
     if (getTriple().getArch() == llvm::Triple::amdgcn) {
       hasFP64 = true;
       hasFMAF = true;
       hasLDEXPF = true;
     }
-
+    auto IsGenericZero = isGenericZero(Triple);
     resetDataLayout(getTriple().getArch() == llvm::Triple::amdgcn ?
-                    DataLayoutStringSI : DataLayoutStringR600);
+                    (IsGenericZero ? DataLayoutStringSIGenericIsZero :
+                        DataLayoutStringSIPrivateIsZero)
+                    : DataLayoutStringR600);
 
-    AddrSpaceMap = &AMDGPUAddrSpaceMap;
+    AddrSpaceMap = IsGenericZero ? &AMDGPUGenericIsZeroMap :
+        &AMDGPUPrivateIsZeroMap;
     UseAddrSpaceMapMangling = true;
   }
 
   uint64_t getPointerWidthV(unsigned AddrSpace) const override {
     if (GPU <= GK_CAYMAN)
       return 32;
 
-    switch(AddrSpace) {
-      default:
-        return 64;
-      case 0:
-      case 3:
-      case 5:
-        return 32;
+    if (AddrSpace == AS.Private || AddrSpace == AS.Local) {
     }
+    return 64;
   }
 
   uint64_t getMaxPointerWidth() const override {
     return getTriple().getArch() == llvm::Triple::amdgcn ? 64 : 32;
   }
 
   const char * getClobbers() const override {
     return "";
public:
   /// \returns If a target requires an address within a target specific address
   /// space \p AddressSpace to be converted in order to be used, then return the
   /// corresponding target specific DWARF address space.
   ///
   /// \returns Otherwise return None and no conversion will be emitted in the
   /// DWARF.
   Optional<unsigned> getDWARFAddressSpace(
       unsigned AddressSpace) const override {
-    switch (AddressSpace) {
-    case 0: // LLVM Private.
-      return 1; // DWARF Private.
-    case 3: // LLVM Local.
-      return 2; // DWARF Local.
-    default:
+    const unsigned DWARF_Private = 1;
+    const unsigned DWARF_Local   = 2;
+    if (AddressSpace == AS.Private) {
+      return DWARF_Private;
+    } else if (AddressSpace == AS.Local) {
+      return DWARF_Local;
+    } else {
       return None;
     }
   }
 
   CallingConvCheckResult checkCallingConvention(CallingConv CC) const override {
     switch (CC) {
       default:
         return CCCR_Warning;
       case CC_C:
       case CC_OpenCLKernel:
         return CCCR_OK;
     }
   }
 
   // In amdgcn target the null pointer in global, constant, and generic
   // address space has value 0 but in private and local address space has
   // value ~0.
   uint64_t getNullPointerValue(unsigned AS) const override {
     return AS == LangAS::opencl_local ? ~0 : 0;
   }
+
+  const AddrSpace AS;
 };
 
 const Builtin::Info AMDGPUTargetInfo::BuiltinInfo[] = {
 #define BUILTIN(ID, TYPE, ATTRS)                \
   { #ID, TYPE, ATTRS, nullptr, ALL_LANGUAGES, nullptr },
 #define TARGET_BUILTIN(ID, TYPE, ATTRS, FEATURE)                               \
   { #ID, TYPE, ATTRS, nullptr, ALL_LANGUAGES, FEATURE },
 #include "clang/Basic/BuiltinsAMDGPU.def"

cfe/trunk/test/CodeGenOpenCL/amdgpu-env-amdgiz.cl

+// RUN: %clang_cc1 %s -O0 -triple amdgcn -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -O0 -triple amdgcn---opencl -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -O0 -triple amdgcn---amdgiz -emit-llvm -o - | FileCheck -check-prefix=GIZ %s
+// RUN: %clang_cc1 %s -O0 -triple amdgcn---amdgizcl -emit-llvm -o - | FileCheck -check-prefix=GIZ %s
+
+// CHECK: target datalayout = "e-p:32:32-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64"
+// GIZ: target datalayout = "e-p:64:64-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64"
+void foo(void) {}
+