[PATCH] D84276: GlobalISel: Don't fail translate on intrinsics with metadata

Mon Jul 27 10:15:34 PDT 2020

ab accepted this revision.
ab added a comment.
This revision is now accepted and ready to land.

In D84276#2172756 <https://reviews.llvm.org/D84276#2172756>, @arsenm wrote:

> In D84276#2172675 <https://reviews.llvm.org/D84276#2172675>, @paquette wrote:
>
> > In rG55d10423a6a1f8286be05651b9c7143ef4e58b22 <https://reviews.llvm.org/rG55d10423a6a1f8286be05651b9c7143ef4e58b22>
> >
> > > [GlobalISel] Don't translate intrinsics with metadata parameters. 
> > >  Some intrinsics take metadata parameters.  These all need custom handling of some form, and cannot possibly be lowered generically to G_INTRINSIC calls with vreg operands. Reject them, instead of hitting an assert later in getOrCreateVReg.
> >
> > Is this still relevant? I guess we're just using custom legalization and selection to handle these nowadays?
> >
> > (@ab do you have opinions here?)
>

This makes sense to me; for our targets we hadn't encountered many intrinsics with metadata, and I see that read/write_register have special opcodes now.  If there are more for which it makes sense to deal with later, this seems totally reasonable.  (but for read/write_register I could see them being emitted as the native copies in irtranslator directly, though I could see that either way)

At this point both of you have more relevant opinions than I do, but this LGTM ;)

> I don’t think it makes any sense to try to use vregs for metadata. It’s like a weird form of immarg and shouldn’t be materialized. The generic intrinsics with metadata arguments should all probably be handled in the IRTranslator into G_* instructions with something else, but target intrinsics should just forward along the metadata operand

Agreed, this all makes sense to me

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

https://reviews.llvm.org/D84276