[PATCH] D55639: GlobalISel: Allow shift amount to be a different type

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

In D55639#1352510 <https://reviews.llvm.org/D55639#1352510>, @arsenm wrote:

> In D55639#1350196 <https://reviews.llvm.org/D55639#1350196>, @aemerson wrote:
>
> > In D55639#1349093 <https://reviews.llvm.org/D55639#1349093>, @arsenm wrote:
> >
> > > In D55639#1348912 <https://reviews.llvm.org/D55639#1348912>, @aemerson wrote:
> > >
> > > > Thanks, this definitely needed to be done. Looks mostly fine, but I also can't be confident on the X86 changes. @igorb @aivchenk can you take a look at this change?
> > >
> > >
> > > Do you have an opinion how how to handle selecting the shift amount type in the combiner?
> >
> >
> > For AArch64 we want shift immediates to be 64 bit in order to select using the imported patterns. I can see 3 options:
> >  a) we ask the target via a hook what the preferred shift immediate type is
> >  b) we iteratively check from i64->i32(maybe -> i16?) using the `isInstUnsupported()` call to see which type combination is legal and pick the first
> >  c) we don't worry about the type and get the legalizer to re-legalize it. I don't think however that the legalizer artifact combiner currently tries to re-legalize the combined instructions. @aditya_nandakumar do you have an opinion?
> >
> > For now, my slight preference would be option b) as its simpler and doesn't try to change the way the legalizer/combiner currently works.
>
>
> This does assume a power of 2 legality


I spoke with Aditya offline and the legalizer combines will actually retry legalization of any instructions generated by the combine. This means we could use option c), and just emit the shift with the default type, and rely on the target's legalization rules to morph it into the best form.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55639/new/

https://reviews.llvm.org/D55639