Page MenuHomePhabricator

[ARM] Adjust calling conventions for MVE vectors.
Needs ReviewPublic

Authored by simon_tatham on Apr 15 2019, 6:00 AM.

Details

Summary

This allows the full set of MVE-relevant vector types to be passed as
arguments in vector registers.

Event Timeline

simon_tatham created this revision.Apr 15 2019, 6:00 AM

Why do we need to treat this differently to NEON? As far as I know the calling convention is the same.

Can this be tested now, or does it need the next patch? In either case, some tests should be added somewhere.

The problem with the existing NEON calling convention code was that it started by pretending all 128-bit vectors are v2f64. But that's the one vector type MVE doesn't have any support for, so there were lots of annoying ISel errors.

Even so, I think you're right that this was the wrong solution. I've managed to get all the tests passing without modifying ARMCallingConv.td at all, by the alternative technique of making v2f64 a legal type in MVE-land even though no actual instructions will usefully operate on one (i.e. everything other than loads and stores are set to Expand). I can fold those changes into other patches in this series, so I think this one can be abandoned.

Hello. I'm a little worried about the implication of making v2f64 legal, from a codegen point of view. My understanding is that that would make a lot of cost calculations wrong at least? And by default the vectoriser would start very happily making v2f64 vectors, which we would have trouble dealing with. I may be mistaken about the ramifications of doing it, but if we were to mark v2f64 as legal just for calling conventions, and it did then hurt us in a lot of other places, that seems like a poor tradeoff to me.

I think that's unlikely to be too much of a problem, because it exactly mimics the state of affairs for AArch32 NEON, which also does not support arithmetic operations on vectors of two f64s. The existing NEON setup code in ARMISelLowering.cpp makes v2f64 a legal type for argument passing purposes but sets most operations to Expand, and my revised version of D60708 (which I seem not to have uploaded yet, oops) just modifies an existing if statement or two so that exactly the same handling applies to MVE.