Page MenuHomePhabricator

[InstCombine] Add an option to disable addrspacecast folding into GEP
AbandonedPublic

Authored by mareko on Dec 31 2017, 8:21 PM.

Details

Summary

AMDGPU users use addrspacecast to zero-extend a 32-bit pointer to 64 bits.
No other instruction is ever used on 32-bit pointers.

AMDGPU doesn't have full support for 32-bit pointers everywhere, so make
sure addrspacecast isn't folded, so that we don't get GEP on 32-bit
pointers.

Event Timeline

mareko created this revision.Dec 31 2017, 8:21 PM

FYI, I'd like to get this into LLVM 6.0 if it's OK.

davide requested changes to this revision.Jan 2 2018, 5:50 AM

I'm not sure having a cl::opt is the best option here.
If you really want to make such a change, can you at least thread this information through TargetTransformInfo rather than using a global option?
Also, for my own curiosity, is this a temporary workaround until AMDGPU grows proper support for 32-bit GEPs or is this an inherent limitation?
If the former, maybe this hack should not go in (or at least we should consider what's the amount of work needed to finish implementing the support)

This revision now requires changes to proceed.Jan 2 2018, 5:50 AM
mareko added a comment.Jan 2 2018, 6:19 AM

I'm not sure having a cl::opt is the best option here.
If you really want to make such a change, can you at least thread this information through TargetTransformInfo rather than using a global option?

Yes.

Also, for my own curiosity, is this a temporary workaround until AMDGPU grows proper support for 32-bit GEPs or is this an inherent limitation?
If the former, maybe this hack should not go in (or at least we should consider what's the amount of work needed to finish implementing the support)

It's possible that this will be a permanent solution, because we don't plan to have full 32-bit support (it would be too much work in the backend for little benefit).

davide added a comment.Jan 2 2018, 6:31 AM

I'm not sure having a cl::opt is the best option here.
If you really want to make such a change, can you at least thread this information through TargetTransformInfo rather than using a global option?

Yes.

Also, for my own curiosity, is this a temporary workaround until AMDGPU grows proper support for 32-bit GEPs or is this an inherent limitation?
If the former, maybe this hack should not go in (or at least we should consider what's the amount of work needed to finish implementing the support)

It's possible that this will be a permanent solution, because we don't plan to have full 32-bit support (it would be too much work in the backend for little benefit).

This is quite unfortunate. I'd like to point out I don't feel particularly comfortable to have this as a long term solution (but, maybe, OK for 6.0 & reverted in trunk).
The contract between the mid-level optimizer and the backend is that the latter should possibly accept everything produced by the former (or, FWIW, error out in some circumstances).
Maybe you can have something in your backend logic that recovers from the fact that AMDGPU doesn't know (and won't be taught) about 32-bit GEPs?
Have you considered something during legalization? [Apologies if I'm off, but I'm not really familiar with the way AMDGPU works, at least not in depth].

mareko added a comment.Jan 2 2018, 7:31 AM

I'm not sure having a cl::opt is the best option here.
If you really want to make such a change, can you at least thread this information through TargetTransformInfo rather than using a global option?

Yes.

Also, for my own curiosity, is this a temporary workaround until AMDGPU grows proper support for 32-bit GEPs or is this an inherent limitation?
If the former, maybe this hack should not go in (or at least we should consider what's the amount of work needed to finish implementing the support)

It's possible that this will be a permanent solution, because we don't plan to have full 32-bit support (it would be too much work in the backend for little benefit).

This is quite unfortunate. I'd like to point out I don't feel particularly comfortable to have this as a long term solution (but, maybe, OK for 6.0 & reverted in trunk).
The contract between the mid-level optimizer and the backend is that the latter should possibly accept everything produced by the former (or, FWIW, error out in some circumstances).
Maybe you can have something in your backend logic that recovers from the fact that AMDGPU doesn't know (and won't be taught) about 32-bit GEPs?
Have you considered something during legalization? [Apologies if I'm off, but I'm not really familiar with the way AMDGPU works, at least not in depth].

GEPs will actually work, but the generated assembly for loads will be worse, thus I'd like to avoid them. A better solution for LLVM 7.0 might be possible.

davide added a comment.Jan 2 2018, 7:35 AM

I'm not sure having a cl::opt is the best option here.
If you really want to make such a change, can you at least thread this information through TargetTransformInfo rather than using a global option?

Yes.

Also, for my own curiosity, is this a temporary workaround until AMDGPU grows proper support for 32-bit GEPs or is this an inherent limitation?
If the former, maybe this hack should not go in (or at least we should consider what's the amount of work needed to finish implementing the support)

It's possible that this will be a permanent solution, because we don't plan to have full 32-bit support (it would be too much work in the backend for little benefit).

This is quite unfortunate. I'd like to point out I don't feel particularly comfortable to have this as a long term solution (but, maybe, OK for 6.0 & reverted in trunk).
The contract between the mid-level optimizer and the backend is that the latter should possibly accept everything produced by the former (or, FWIW, error out in some circumstances).
Maybe you can have something in your backend logic that recovers from the fact that AMDGPU doesn't know (and won't be taught) about 32-bit GEPs?
Have you considered something during legalization? [Apologies if I'm off, but I'm not really familiar with the way AMDGPU works, at least not in depth].

GEPs will actually work, but the generated assembly for loads will be worse, thus I'd like to avoid them. A better solution for LLVM 7.0 might be possible.

Cool. After you update the patch (and we reach consensus), I guess it's not unreachable to commit this to trunk, back port to 6, then revert and start working on a better solution for the 7.0 timeframe.
I do understand we all have deadlines at times :)

aprantl added inline comments.Jan 2 2018, 9:46 AM
include/llvm/IR/Value.h
512

The \brief is redundant, we use autobrief now.

mareko abandoned this revision.Jan 2 2018, 12:29 PM

Abandoning. The new plan is that we'll do it the right way in AMDGPU now.