[AArch64] Properly validate the reciprocal estimation

[evandro]

Summary

Add check for legal data types when expanding into a Newton series.

[t.p.northover]

I think you forgot to add llvm-commits as a subscriber (that's what makes it get sent to the list, not the repository). Also tests.

[evandro]

@t.p.northover,

WRT the tests, I'd appreciate some help. In some cases, lowering would fail because the square root was being expanded into the series for MVT::v4f64, which is obviously not supported by AArch64. I'm at a loss as to why such data type, unsupported by the target, percolated through to the backend.

Would concocting the bytecode using such data type be a sensible test?

On another note, as I'm not very familiar with the latest versions of ARMv8, should FP16 be considered here?

Thank you.

[t.p.northover]

Would concocting the bytecode using such data type be a sensible test?

That's definitely a valid way to write the test if you can get it to crash like that. We should be accepting and legalizing all IR.

On another note, as I'm not very familiar with the latest versions of ARMv8, should FP16 be considered here?

Yes. f16, v4f16 and v8f16 are valid for both if Subtarget->hasFullFP16().

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

4579

Why the extra brackets?

[t.p.northover]

Also, probably v1i64 (a bit of a silly type, but it does exist and should work).

[t.p.northover]

Sorry, v1f64.

[evandro]

In D22267#481792, @t.p.northover wrote:

Also, probably v1i64 (a bit of a silly type, but it does exist and should work).

Whaaat? Please, explain.

[evandro]

In D22267#481801, @t.p.northover wrote:

Sorry, v1f64.

Ah, OK. How about v1f32?

[evandro]

@t.p.northover,

Please, correct me if I'm wrong, but there's neither FRECPE nor FRSQRTE insts in ARMv8.1, are there?

[t.p.northover]

Ah, OK. How about v1f32?

I wouldn't worry about that, I don't think it's used nearly as frequently. The v1f64 issue comes up because that's how arm_neon.h represents some vectors.

Please, correct me if I'm wrong, but there's neither FRECPE nor FRSQRTE insts in ARMv8.1, are there?

That's right. Full support for _Float16 is part of v8.2 I believe, but tracked as a separate feature in LLVM: "fullfp16" if you're doing -mattr.

[evandro]

In D22267#482029, @t.p.northover wrote:

That's right. Full support for _Float16 is part of v8.2 I believe, but tracked as a separate feature in LLVM: "fullfp16" if you're doing -mattr.

I don't have the v8.2 documentation at hand, so, please indulge me, does it support both FRECPE and FRSQRTE for _Float16?

[t.p.northover]

I don't have the v8.2 documentation at hand, so, please indulge me, does it support both FRECPE and FRSQRTE for _Float16?

It does:

$ echo "frecpe v0.4h, v1.4h" | llvm-mc -triple aarch64 -mattr=+fullfp16 -show-inst
frecpe v0.4h, v1.4h           // <MCInst #859 FRECPEv4f16
                              //  <MCOperand Reg:40>
                              //  <MCOperand Reg:41>>
$ echo "frsqrte v0.4h, v1.4h" | llvm-mc -triple aarch64 -mattr=+fullfp16 -show-inst
frsqrte	v0.4h, v1.4h            // <MCInst #934 FRSQRTEv4f16
                                //  <MCOperand Reg:40>
                                //  <MCOperand Reg:41>>

(plus similar results for scalar operations).

[evandro]

Thanks, @t.p.northover.

[evandro]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

4579

Oops!

[evandro]

Add validation for the _Float16 types.

[evandro]

Update existing test cases to include the error vector found in the wild.

[t.p.northover]

Thanks Evandro! I think this looks fine now.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

     return SDValue();
 
   EVT VT = Operand.getValueType();
+  if (VT != MVT::f64 && VT != MVT::v1f64 && VT != MVT::v2f64 &&
+      VT != MVT::f32 && VT != MVT::v1f32 &&
+      VT != MVT::v2f32 && VT != MVT::v4f32 &&
+      (!ST.hasFullFP16() ||
+       (VT != MVT::f16 && VT != MVT::v4f16 && VT != MVT::v8f16)))
+    return SDValue();
 
   std::string RecipOp;
   RecipOp = Opcode == (AArch64ISD::FRECPE) ? "div": "sqrt";