[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

What are these values used for?

2057

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

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

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

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

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

Is there a reason this is no longer const?

2024

Same as above.

2357

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

[yaxunl]

lib/Basic/Targets.cpp

2015

sorry, my omission. Will fix it when commit.

2357

Yes. will do.

include/clang/Driver/Options.td

   Group<m_Group>,
   HelpText<"Generate additional code for specified <version> of debugger ABI (AMDGPU only)">,
   MetaVarName<"<version>">;
+def mnew_addr : Flag<["-"], "mnew-addr">,
+  Group<m_amdgpu_Features_Group>,
+  HelpText<"Use new address space mapping for AMDGPU target">;
 
 def mvsx : Flag<["-"], "mvsx">, Group<m_ppc_Features_Group>;
 def mno_vsx : Flag<["-"], "mno-vsx">, Group<m_ppc_Features_Group>;

lib/Basic/Targets.cpp

   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
-};
-
 // If you edit the description strings, make sure you update
 // getPointerWidthV().
 

   "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 DataLayoutStringSIOld =
   "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 DataLayoutStringSINew =
+  "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;
+    bool UseNew;
+    AddrSpace(bool UseNew_ = false){
+      reset(UseNew_);
+    }
+    void reset(bool UseNew_) {
+      UseNew = UseNew_;
+      if (UseNew) {
+        Generic   = 0;
+        Global    = 1;
+        Local     = 3;
+        Constant  = 4;
+        Private   = 5;
+      } else {
+        Generic   = 4;
+        Global    = 1;
+        Local     = 3;
+        Constant  = 2;
+        Private   = 0;
+      }
+    }
+    const LangAS::Map *getMap() {
+      static LangAS::Map OldMap = {
+          1,  // opencl_global
+          3,  // opencl_local
+          2,  // opencl_constant
+          4,  // opencl_generic
+          1,  // cuda_device
+          2,  // cuda_constant
+          3   // cuda_shared
+      };
+      static LangAS::Map NewMap = {
+          1,  // opencl_global
+          3,  // opencl_local
+          4,  // opencl_constant
+          0,  // opencl_generic
+          1,  // cuda_device
+          4,  // cuda_constant
+          3   // cuda_shared
+      };
+      return UseNew ? &NewMap : &OldMap;
+    }
+  };
+
   /// \brief The GPU profiles supported by the AMDGPU target.
   enum GPUKind {
     GK_NONE,

   bool hasFMAF:1;
   bool hasLDEXPF:1;
   bool hasFullSpeedFP32Denorms:1;
+  bool UseNewAddrMap:1;
 
   static bool isAMDGCN(const llvm::Triple &TT) {
     return TT.getArch() == llvm::Triple::amdgcn;

       hasFP64(false),
       hasFMAF(false),
       hasLDEXPF(false),
-      hasFullSpeedFP32Denorms(false){
+      hasFullSpeedFP32Denorms(false),
+      UseNewAddrMap(false){
     if (getTriple().getArch() == llvm::Triple::amdgcn) {
       hasFP64 = true;
       hasFMAF = true;
       hasLDEXPF = true;
     }
+    // At this stage, Opt.Features has not been populated yet.
+    for (auto &I:Opts.FeaturesAsWritten) {
+      if (I == "+new-addr") {
+        UseNewAddrMap = true;
+      } else if (I == "-new-addr") {
+        UseNewAddrMap = false;
+      }
+    }
+    AS.reset(UseNewAddrMap);
 
     resetDataLayout(getTriple().getArch() == llvm::Triple::amdgcn ?
-                    DataLayoutStringSI : DataLayoutStringR600);
+                    (UseNewAddrMap ? DataLayoutStringSINew :
+                        DataLayoutStringSIOld)
+                    : DataLayoutStringR600);
 
-    AddrSpaceMap = &AMDGPUAddrSpaceMap;
+    AddrSpaceMap = AS.getMap();
     UseAddrSpaceMapMangling = true;
   }
 

     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 32;
     }
+    return 64;
   }
 
   uint64_t getMaxPointerWidth() const override {

   /// 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;
     }
   }

   uint64_t getNullPointerValue(unsigned AS) const override {
     return AS == LangAS::opencl_local ? ~0 : 0;
   }
+
+  AddrSpace AS;
 };
 
 const Builtin::Info AMDGPUTargetInfo::BuiltinInfo[] = {

test/CodeGenOpenCL/amdgpu-new-addr.cl

+// RUN: %clang_cc1 %s -O0 -triple amdgcn -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -O0 -triple amdgcn -target-feature "+new-addr" -emit-llvm -o - | FileCheck -check-prefix=NEW %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"
+// NEW: 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) {}
+