Page MenuHomePhabricator

[AMDGPU] Switch address space mapping by triple environment
ClosedPublic

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

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

yaxunl created this revision.Mar 21 2017, 2:20 PM

getDWARFAddressSpace also needs to be updated.

yaxunl edited the summary of this revision. (Show Details)Mar 21 2017, 2:29 PM
rampitec edited edge metadata.Mar 21 2017, 2:30 PM

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.

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?

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 edited edge metadata.Mar 21 2017, 3:43 PM

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?

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 updated this revision to Diff 92587.Mar 21 2017, 6:57 PM

Fix getDWARFAddressSpace.

yaxunl updated this revision to Diff 92674.Mar 22 2017, 11:57 AM
yaxunl edited the summary of this revision. (Show Details)
tstellar added inline comments.
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?

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 added inline comments.Mar 22 2017, 12:16 PM
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.

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

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

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 updated this revision to Diff 92717.Mar 22 2017, 2:40 PM
yaxunl edited the summary of this revision. (Show Details)

Rename the triple and variable names.

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

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

Thanks.

yaxunl updated this revision to Diff 92977.Mar 24 2017, 11:02 AM

Revised by Tony's suggestions.

t-tye requested changes to this revision.Mar 24 2017, 11:13 AM

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.

This revision now requires changes to proceed.Mar 24 2017, 11:13 AM

Hi Tony,

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

yaxunl updated this revision to Diff 92997.Mar 24 2017, 1:36 PM
yaxunl edited edge metadata.
yaxunl retitled this revision from [AMDGPU] Add new address space mapping to [AMDGPU] Switch address space mapping by triple environment.

Use triple environment name.

t-tye accepted this revision.Mar 24 2017, 5:16 PM

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?

This revision is now accepted and ready to land.Mar 24 2017, 5:16 PM
yaxunl added inline comments.Mar 24 2017, 6:42 PM
lib/Basic/Targets.cpp
2015 ↗(On Diff #92997)

sorry, my omission. Will fix it when commit.

2357 ↗(On Diff #92997)

Yes. will do.

This revision was automatically updated to reflect the committed changes.