[AMDGPU] Switch data layout by triple environment

[yaxunl]

Summary

Switch data layout by target triple environment amdgiz and amdgizcl indicating using of an address space mapping in which generic address space is 0.

amdgiz is for non-OpenCL environment where generic address space is 0.

amdgizcl is for OpenCL environment where generic address space is 0.

This is mainly to allow upstreaming Clang changes for changing address space mapping https://reviews.llvm.org/D31210 since the Clang data layout needs to match the backend data layout.

[yaxunl]

Add a test.

[arsenm]

This can't be a subtarget feature. This needs to be a global option since this is a property of the module

[rampitec]

I believe it is much more than just adding feature. At the very least it should change addrspace enum and potentially its uses.

[yaxunl]

In D31211#706894, @rampitec wrote:

I believe it is much more than just adding feature. At the very least it should change addrspace enum and potentially its uses.

This is just a start point for upstreaming the Clang changes. Further changes will follow.

[yaxunl]

In D31211#706892, @arsenm wrote:

This can't be a subtarget feature. This needs to be a global option since this is a property of the module

By convention, Clang use -m options for target specific options, e.g. -mmmx. Clang translates them to target feature string by dropping -m. When TargetInfo is constructed in Clang, only TargetOption is available. I don't see a better way to define a Clang option which is only used by AMDGPU target.

[rampitec]

In D31211#706913, @yaxunl wrote:

In D31211#706894, @rampitec wrote:

I believe it is much more than just adding feature. At the very least it should change addrspace enum and potentially its uses.

This is just a start point for upstreaming the Clang changes. Further changes will follow.

From this it is hard to tell what will you do with the static addrspace enum...

[yaxunl]

In D31211#707021, @rampitec wrote:

In D31211#706913, @yaxunl wrote:

In D31211#706894, @rampitec wrote:

I believe it is much more than just adding feature. At the very least it should change addrspace enum and potentially its uses.

This is just a start point for upstreaming the Clang changes. Further changes will follow.

From this it is hard to tell what will you do with the static addrspace enum...

For now this is mainly to provide a matching data layout for Clang, otherwise when Clang emit llvm for AMDGPU there will be warnings.

We cannot upstream everything at the same time. That will be too big to review. The plan is to let Clang generates expected LLVM IR first, then bring in the backend change.

By this patch, we can also facilitate upstreaming the changes for letting alloca returning private pointer. What we need to do is for the old address space mapping we let alloca return pointer in address space 0, and for the new address space mapping, we let alloca return pointer in address space 5. This way, we could upstream the alloca change without breaking anything. In the final stage hopefully we only need minor changes to switch to the new address space mapping.

[yaxunl]

In D31211#706970, @yaxunl wrote:

In D31211#706892, @arsenm wrote:

This can't be a subtarget feature. This needs to be a global option since this is a property of the module

By convention, Clang use -m options for target specific options, e.g. -mmmx. Clang translates them to target feature string by dropping -m. When TargetInfo is constructed in Clang, only TargetOption is available. I don't see a better way to define a Clang option which is only used by AMDGPU target.

In Clang codegen, when clang creates a module, it will use data layout specified by Clang's TargetInfo, then it will create a target machine and query the data layout returned by the target machine. If these two data layout mismatch, it will emit a warning. When Clang queries the data layout of the target machine, it only depends on the properties of the target machine itself. If we want to tell the target machine to use a specific address space mapping, we need to convey that information through the arguments of the constructor of the target machine, e.g. target triple, target option or feature string. It seems to me that feature string is the reasonable way to convey this information.

Can you elaborate on your comments about global option? A global option of what? The AMDGPU backend, or the LLVM module? Thanks.

[arsenm]

In D31211#707611, @yaxunl wrote:

In D31211#706970, @yaxunl wrote:

In D31211#706892, @arsenm wrote:

This can't be a subtarget feature. This needs to be a global option since this is a property of the module

By convention, Clang use -m options for target specific options, e.g. -mmmx. Clang translates them to target feature string by dropping -m. When TargetInfo is constructed in Clang, only TargetOption is available. I don't see a better way to define a Clang option which is only used by AMDGPU target.

In Clang codegen, when clang creates a module, it will use data layout specified by Clang's TargetInfo, then it will create a target machine and query the data layout returned by the target machine. If these two data layout mismatch, it will emit a warning. When Clang queries the data layout of the target machine, it only depends on the properties of the target machine itself. If we want to tell the target machine to use a specific address space mapping, we need to convey that information through the arguments of the constructor of the target machine, e.g. target triple, target option or feature string. It seems to me that feature string is the reasonable way to convey this information.

Can you elaborate on your comments about global option? A global option of what? The AMDGPU backend, or the LLVM module? Thanks.

You can add an arbitrary environment type string to the end of the triple you could check

[yaxunl]

In D31211#707627, @arsenm wrote:

In D31211#707611, @yaxunl wrote:

In D31211#706970, @yaxunl wrote:

In D31211#706892, @arsenm wrote:

This can't be a subtarget feature. This needs to be a global option since this is a property of the module

By convention, Clang use -m options for target specific options, e.g. -mmmx. Clang translates them to target feature string by dropping -m. When TargetInfo is constructed in Clang, only TargetOption is available. I don't see a better way to define a Clang option which is only used by AMDGPU target.

In Clang codegen, when clang creates a module, it will use data layout specified by Clang's TargetInfo, then it will create a target machine and query the data layout returned by the target machine. If these two data layout mismatch, it will emit a warning. When Clang queries the data layout of the target machine, it only depends on the properties of the target machine itself. If we want to tell the target machine to use a specific address space mapping, we need to convey that information through the arguments of the constructor of the target machine, e.g. target triple, target option or feature string. It seems to me that feature string is the reasonable way to convey this information.

Can you elaborate on your comments about global option? A global option of what? The AMDGPU backend, or the LLVM module? Thanks.

You can add an arbitrary environment type string to the end of the triple you could check

Sounds great. I will try that. Thanks.

[yaxunl]

Use triple environment to indicate new address space mapping used.

[arsenm]

include/llvm/ADT/Triple.h

202–203 ↗

(On Diff #92672)

I don't think we should be adding new entries for a temporary step. I think the triple still preserves whatever you give it, so can you check getEnvironmentName()?

[yaxunl]

Rename the triple environment.

[t-tye]

LGTM

[arsenm]

include/llvm/ADT/Triple.h

202–203 ↗

(On Diff #92710)

Can you not include these and just directly check getEnvironmentName? We should not be adding temporary junk here

[yaxunl]

Revised by Matt's comments.

llvm/trunk/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

static StringRef computeDataLayout(const Triple &TT) {
   if (TT.getArch() == Triple::r600) {
     // 32-bit pointers.
     return "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";
   }
 
   // 32-bit private, local, and region pointers. 64-bit global, constant and
   // flat.
+  if (TT.getEnvironmentName() == "amdgiz" ||
+      TT.getEnvironmentName() == "amdgizcl")
+    return "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";
   return "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";
 }
 
 LLVM_READNONE
 static StringRef getGPUOrDefault(const Triple &TT, StringRef GPU) {
   if (!GPU.empty())
     return GPU;
 
   // HSA only supports CI+, so change the default GPU to a CI for HSA.

llvm/trunk/test/CodeGen/AMDGPU/env-amdgiz.ll

+; RUN: llc -march=amdgcn -mtriple=amdgcn-amd-amdhsa-amdgiz -verify-machineinstrs < %s
+; Just check the target feature and data layout is accepted without error.
+
+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"
+target triple = "amdgcn-amd-amdhsa-amdgiz"
+
+define void @foo() {
+entry:
+  ret void
+}
+

llvm/trunk/test/CodeGen/AMDGPU/env-amdgizcl.ll

+; RUN: llc -march=amdgcn -mtriple=amdgcn-amd-amdhsa-amdgizcl -verify-machineinstrs < %s
+; Just check the target feature and data layout is accepted without error.
+
+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"
+target triple = "amdgcn-amd-amdhsa-amdgizcl"
+
+define void @foo() {
+entry:
+  ret void
+}
+