[PATCH] D41652: [InstCombine] Add an option to disable addrspacecast folding into GEP

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

In https://reviews.llvm.org/D41652#965836, @mareko wrote:

> In https://reviews.llvm.org/D41652#965806, @davide wrote:
>
> > In https://reviews.llvm.org/D41652#965797, @mareko wrote:
> >
> > > In https://reviews.llvm.org/D41652#965765, @davide wrote:
> > >
> > > > 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 :)


https://reviews.llvm.org/D41652