Page MenuHomePhabricator

[GlobalISel][AArch64] Always fall back on aarch64.neon.addp.*

Authored by paquette on Mar 6 2019, 4:44 PM.



If you have an intrinsic that doesn't have a 1:1 mapping with an instruction, and the types passed into that intrinsic aren't fully defined by their register banks, GISel can't actually map it to the right instruction today. As far as I know, addp is the only such intrinsic in AArch64.

Introduce a hacky workaround in the AArch64 legalizer to make sure we always fall back on this intrinsic. Goal here is to avoid impacting other backends as much as possible while we work towards a proper solution for the problem.

Also loosen some constraints on the legality of instructions with custom legalization, and factor out an assert. Before, we assumed that legalization would always *modify* an instruction *into* a legal state (or it'd be listed as legal based off the input LLTs). Since LLTs aren't sufficient here, this assumption falls over.

Also, we had the assumption that we'd be passing along LLTs into apply(), which isn't true here either.

Diff Detail


Event Timeline

paquette created this revision.Mar 6 2019, 4:44 PM
kristof.beyls added inline comments.Mar 7 2019, 1:27 AM
518–527 ↗(On Diff #189616)

Over time, more intrinsics get added.
So, making an exception based on a specific intrinsic name here seems like it's a time bomb waiting to go off when new intrinsics get added which have the same properties resulting in a silent codegen faults.
Wouldn't it be possible to check for the properties of the intrinsics that result in incorrect code generation and check on those properties rather than explicitly blacklist based on name?

aemerson added inline comments.Mar 7 2019, 8:55 AM
518–527 ↗(On Diff #189616)

You're right, this is not a solution that fixes the real issue. It's a workaround that we plan to be in place until we settle on a way to fix this in the general case.

Unfortunately, barring disallowing all potentially int/fp type overloaded intrinsics during translation, we don't have a good way to detect these problematic ones automatically. This issue happens when no other attribute of the intrinsic call disambiguates the selected opcode, so element size etc isn't enough. Knowing that isn't generally possible unless we analyze all the selection patterns.

If you like, we could expand this fallback to something more general, up until falling back to all neon intrinsics. However I think that's probably unnecessary.

paquette marked an inline comment as done.Mar 7 2019, 9:35 AM
paquette added inline comments.
518–527 ↗(On Diff #189616)

One way I can think of making this more general that might work is to check if both of the following hold:

  1. The intrinsic is overloaded
  2. There is at least one operand being passed into the intrinsic that is a vector/aggregate type

I think that both of these being true is sufficient to cover all such cases. Case (1) covers the fact that we have an intrinsic with more than one instruction associated with it in the first case.

Case (2) ought to work because for any scalar type, the register bank ought to cover for the missing type info. With vectors, the type info isn't implied by the register bank, and so that's where I think we're going wrong.

arsenm added inline comments.Mar 7 2019, 11:27 AM
81–84 ↗(On Diff #189616)

Why is this necessary? This should hit the default in the switch. You could alternatively add an explicit case if you want to keep the comment

arsenm added inline comments.Mar 7 2019, 11:28 AM
448–453 ↗(On Diff #189616)

This is really isLegalOrCustom. It may be useful to have both

38 ↗(On Diff #189616)

Should this be added separately by D31359?

paquette marked 2 inline comments as done.Mar 7 2019, 3:03 PM
paquette added inline comments.
81–84 ↗(On Diff #189616)

Because if you hand it a G_INTRINSIC, Q.Types ends up with a size of 0 (for some reason), so you crash.

38 ↗(On Diff #189616)

I'm not sure what the status of that patch is, but if it's going to land, sure.

Otherwise, I guess we get everything through custom legalization anyway, so I'm not sure if it matters.

aemerson added inline comments.Mar 7 2019, 3:04 PM
38 ↗(On Diff #189616)

I don't think we need that patch any more if we can do this with legalizeCustom, it's a subset of this functionality.

aemerson accepted this revision.Mar 7 2019, 3:38 PM

LGTM, can you rename the test to something more specific like "fallback-ambiguous-addp-intrinsic.mir"

This revision is now accepted and ready to land.Mar 7 2019, 3:38 PM
paquette updated this revision to Diff 190149.Mar 11 2019, 1:45 PM

addressing minor comments before committing

  • Changes to isLegal -> isLegalOrCustom
  • Rename test
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2019, 1:45 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
This revision was automatically updated to reflect the committed changes.