[PATCH] D73446: AMDGPU/GlobalISel: Legalize a16 images

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In D73446#1846283 <https://reviews.llvm.org/D73446#1846283>, @nhaehnle wrote:

> In D73446#1842457 <https://reviews.llvm.org/D73446#1842457>, @arsenm wrote:
>
> > In D73446#1842443 <https://reviews.llvm.org/D73446#1842443>, @arsenm wrote:
> >
> > > In D73446#1842422 <https://reviews.llvm.org/D73446#1842422>, @nhaehnle wrote:
> > >
> > > > Why is this conceptually a legalization rather than part of the instruction selection?
> > >
> > >
> > > The register layout packing/unpacking code should be exposed to the legalizer and combines. Doing it later means it could possibly be in a waterfall loop, and won't benefit from the combines on the pack/unpack code.
> >
> >
> > More abstractly, selection should operate on the types expected for the final instruction. It's the legalizer's job to get registers that are the right type/size
>
>
> The part that makes me uncomfortable here and I couldn't quite put a finger on initially is that it implies a change in the semantics of the intrinsic. What this and related changes are doing is implicitly changing the design such that the intrinsics mean something different before vs. after legalization. That is not what legalization is usually supposed to do.


Ideally we should have separate intermediate image opcodes. However, I have no interest in creating another giant set of image opcodes that will need more searchable tables. I don't think it's really a semantic change, and it should be possible to go backwards from the legalized intrinsic to what it was originally. What I am worried about is what happens if you run the legalizer twice, which should be OK but I'm ignoring this problem for now until I have the full selection working. Two options I've considered are re-using some of the extra bits in one of the immediate arguments to encode how it's been changed, or to use a variadic wrapper instruction which will just capture the intrinsic ID and how it was legalized.

> I see your point about combiner passes -- so could we perhaps just select the image instructions early instead? It's not like we have very interesting things happening for image instructions in the ISel patterns anyway, and one of the benefits of the GlobalISel infrastructure is that it's supposed to be flexible enough for stuff like that...

I think the only reasonable place outside of selection to do this would be in RegBankSelect/applyMappingImpl, which I did consider before going with the legalizer. We still need to RegBankSelect the intrinsics, and possibly move them into a waterfall loop, and I think introducing real register class constraints earlier is generally undesirable. Eventually, we should run some combiner pass after which should take care of packing code. We could theoretically trick RegBankSelect into handling the selected instructions, but that would also be pretty disgusting. Another small issue with this is it sort of implies making the full set of legalization artifacts legal for <3 x f16> cases. I started working towards this in D72639 <https://reviews.llvm.org/D72639>, but I don't really like it and would prefer if these were all legalized to <4 x s16> during legalization. I think we could use the artifact combiner to eliminate the illegal register types in RegBankSelect, but I don't think that should be mandatory. We claim some of these these are legal today, but that's mostly a hack to deal with missing features in the legalizer.


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

https://reviews.llvm.org/D73446