Page MenuHomePhabricator

GlobalISel: Don't fail translate on intrinsics with metadata
ClosedPublic

Authored by arsenm on Jul 21 2020, 4:32 PM.

Diff Detail

Event Timeline

arsenm created this revision.Jul 21 2020, 4:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2020, 4:32 PM

In 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?)

In 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?)

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

ab accepted this revision.Jul 27 2020, 10:15 AM

In 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

This revision is now accepted and ready to land.Jul 27 2020, 10:15 AM
In D84276#2176372, @ab wrote:

(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)

I actually did this initially when I added support for these, but that doesn't work for arm for some reason

arsenm updated this revision to Diff 281075.Jul 27 2020, 4:09 PM

Apparently the metadata can be not MDNode in the unhandled constrained FP intrinsics