Page MenuHomePhabricator

[ARM] MVE: allow soft-float ABI to pass vector types.
ClosedPublic

Authored by simon_tatham on Jun 28 2019, 9:33 AM.

Details

Summary

Passing a vector type over the soft-float ABI involves it being split
into four GPRs, so the first thing that has to happen at the start of
the function is to recombine those into a vector register. The ABI
types all vectors as v2f64, so we need to support BUILD_VECTOR for
that type, which I do in this patch by allowing it to be expanded in
terms of INSERT_VECTOR_ELT, and writing an ISel pattern for that in
turn. Similarly, I provide a rule for EXTRACT_VECTOR_ELT so that a
returned vector can be marshalled back into GPRs.

While I'm here, I've also legalized ISD::UNDEF for all vector types,
because I noticed it was being expanded into a BUILD_VECTOR with
explicit zero inputs, which seems like a waste of effort compared to
the optimal handling of 'just do nothing'.

Diff Detail

Repository
rL LLVM

Event Timeline

simon_tatham created this revision.Jun 28 2019, 9:33 AM
dmgreen added inline comments.Jun 30 2019, 12:39 PM
llvm/lib/Target/ARM/ARMISelLowering.cpp
268 ↗(On Diff #207075)

What exactly does making UNDEF legal do? Is there a universal pattern to just ignore it? Why is that not the default?

299 ↗(On Diff #207075)

Also if we make vector float and longs undef legal, should we be doing the same for integer vectors?

6682 ↗(On Diff #207075)

My understanding is that this is (at least part of) the fix for some of the inefficient codegen we see in many of the mve float expansion tests. I would expect a lot of tests to need updating because of it. You can hopefully just run the update script on all the mve-* files.

llvm/test/CodeGen/Thumb2/mve-soft-float-abi.ll
2 ↗(On Diff #207075)

I think this should be thumbv8.1m.main-none-eabi.

4 ↗(On Diff #207075)

Can you add some float and half tests too. They should be the same (in terms of calling convention), but are easy enough to add tests for.

simon_tatham marked 2 inline comments as done.Jul 1 2019, 5:32 AM
simon_tatham added inline comments.
llvm/lib/Target/ARM/ARMISelLowering.cpp
268 ↗(On Diff #207075)

The observable effect of having UNDEF not be legal is this kind of thing you can see in the existing expected results for mve-fmath.ll:

sqrt_float32_t:
@ %bb.0: @ %entry
  vsqrt.f32 s4, s0
  movs r0, #0
  vsqrt.f32 s6, s1
  vsqrt.f32 s8, s2
  vsqrt.f32 s10, s3
  vdup.32 q0, r0
  vmov.f32 s0, s4
  vmov.f32 s1, s6
  vmov.f32 s2, s8
  vmov.f32 s3, s10
  bx lr

in which the movs r0,#0 and vdup.32 q0,r0 pair are filling q0 with all zeroes, which is pointless because the next four instructions overwrite every part of q0 in any case. That happens because an ISD::UNDEF for that vector type is not regarded as legal, and the fallback lowering turns it into 'make a vector of zeroes'.

I have to suppose that UNDEF is legal by default and does the obvious thing – it's just that when we use the blunt instrument of setAllExpand, that sets it to Expand along with everything else, and we have to remember to put it back again.

299 ↗(On Diff #207075)

I think that's not necessary, because we didn't make it illegal in the first place by calling setAllExpand.

Revised patch is intended to apply after D63938 rather than before.

Moved the re-enablement of ISD::UNDEF into setAllExpand itself
(after the other patch started centralising things in there), because
that way it will reliably be turned back on for any type where we
turned it off in the first place.

Also fixed the bogus triple, added some soft-float-abi tests for other
types, and updated a load of tests using update_llc_test_checks.

In order to get the new tests for handling vectors of floats to pass,
I also had to legalize INSERT_VECTOR_ELT with the type set to the
vector's element type, rather than the type of the vector as a
whole, because that seems to be how it gets queried in some situations
(particularly, if the scalar operand is of a floating type that needs
promotion, like f16).

dmgreen accepted this revision.Jul 2 2019, 4:10 AM

Nice one. LGTM

llvm/lib/Target/ARM/ARMISelLowering.cpp
268 ↗(On Diff #207075)

OK that makes sense. It should be legal anyway!

llvm/test/CodeGen/Thumb2/mve-soft-float-abi.ll
59 ↗(On Diff #207362)

Are these auto-generated? It doesn't show the expanded form because it doesn't have a unique check-prefix? I thought this usually gave an error.

This revision is now accepted and ready to land.Jul 2 2019, 4:10 AM
This revision was automatically updated to reflect the committed changes.
simon_tatham marked an inline comment as done.Jul 2 2019, 4:27 AM
simon_tatham added inline comments.
llvm/test/CodeGen/Thumb2/mve-soft-float-abi.ll
59 ↗(On Diff #207362)

I don't know about that – I just reran update_llc_test_checks on this file and it left it alone without complaint.

I deliberately left the expected results for the software-FP expanded form out of the test because it's absolutely enormous, and even more than most of these tests, full of arbitrary decisions about what order to do things in. I feel as if you'd be forever regenerating it on every tiny perturbation of this area of the code, and every time you did, it would be impossible to manually confirm the correctness of the new version without making mistakes...