Page MenuHomePhabricator

[AMDGPU] Switch data layout by triple environment
ClosedPublic

Authored by yaxunl on Mar 21 2017, 2:26 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

yaxunl created this revision.Mar 21 2017, 2:26 PM
yaxunl updated this revision to Diff 92547.Mar 21 2017, 2:28 PM

Add a test.

arsenm edited edge metadata.Mar 21 2017, 2:34 PM

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

rampitec edited edge metadata.Mar 21 2017, 2:35 PM

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

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.

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.

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...

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.

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.

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

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 updated this revision to Diff 92672.Mar 22 2017, 11:53 AM
yaxunl retitled this revision from [AMDGPU] Add target feature new-addr to [AMDGPU] Add target triple environment amdnas and amdnascl.
yaxunl edited the summary of this revision. (Show Details)

Use triple environment to indicate new address space mapping used.

arsenm added inline comments.Mar 22 2017, 12:35 PM
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 updated this revision to Diff 92710.Mar 22 2017, 2:26 PM
yaxunl retitled this revision from [AMDGPU] Add target triple environment amdnas and amdnascl to [AMDGPU] Add target triple environment amdgiz and amdgizcl.
yaxunl edited the summary of this revision. (Show Details)

Rename the triple environment.

t-tye added a subscriber: t-tye.Mar 22 2017, 6:40 PM
tony-tye removed a subscriber: tony-tye.Mar 22 2017, 6:47 PM
t-tye accepted this revision.Mar 24 2017, 11:07 AM

LGTM

This revision is now accepted and ready to land.Mar 24 2017, 11:07 AM
arsenm added inline comments.Mar 24 2017, 11:12 AM
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

arsenm requested changes to this revision.Mar 24 2017, 11:12 AM
This revision now requires changes to proceed.Mar 24 2017, 11:12 AM
yaxunl updated this revision to Diff 92995.Mar 24 2017, 1:31 PM
yaxunl edited edge metadata.
yaxunl retitled this revision from [AMDGPU] Add target triple environment amdgiz and amdgizcl to [AMDGPU] Switch data layout by triple environment.
yaxunl edited the summary of this revision. (Show Details)

Revised by Matt's comments.

This revision was automatically updated to reflect the committed changes.