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

Tue Jul 2 04:10:25 PDT 2019

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

Nice one. LGTM

================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:268
     setOperationAction(ISD::STORE, VT, Legal);
+    setOperationAction(ISD::UNDEF, VT, Legal);

----------------
simon_tatham wrote:
> dmgreen wrote:
> > What exactly does making UNDEF legal do? Is there a universal pattern to just ignore it? Why is that not the default?
> 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.
OK that makes sense. It should be legal anyway!

================
Comment at: llvm/test/CodeGen/Thumb2/mve-soft-float-abi.ll:59
+; CHECK-FP-NEXT:    mov r0, sp
+; CHECK-FP-NEXT:    vldrw.u32 q1, [r0]
+; CHECK-FP-NEXT:    vadd.f16 q0, q0, q1
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63937