[PATCH] D114946: [AArch64] Add instruction selection for strict FP

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Sorry for the delay - these patch is quite big and some of the details in constrained intrinsics can be.. subtle. It makes them difficult to review.

> The best I've been able to do is break out the parts related to fp16 legalization and lowering out into a separate patch, which I'll have ready later today.

I'm not sure I see why all these parts need to be in a single patch. Smaller patches are much more preferable for review. Is it because the entire test file needs to be made to work at the same time? That file looks like it's testing too many separate intrinsics.

But I've tried to take a look through. Comments inline



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:696
+        ISD::FROUND, ISD::FROUNDEVEN, ISD::FMINNUM, ISD::FMAXNUM, ISD::FMINIMUM,
+        ISD::FMAXIMUM, ISD::LROUND, ISD::LLROUND, ISD::LRINT, ISD::LLRINT,
+        ISD::STRICT_FFLOOR, ISD::STRICT_FNEARBYINT, ISD::STRICT_FCEIL,
----------------
Was LLROUND of f16 previously Legal by default? So this doesn't alter anything by explicitly marking it?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:977
     // silliness like this:
-    setOperationAction(ISD::FABS, MVT::v1f64, Expand);
-    setOperationAction(ISD::FADD, MVT::v1f64, Expand);
-    setOperationAction(ISD::FCEIL, MVT::v1f64, Expand);
-    setOperationAction(ISD::FCOPYSIGN, MVT::v1f64, Expand);
-    setOperationAction(ISD::FCOS, MVT::v1f64, Expand);
-    setOperationAction(ISD::FDIV, MVT::v1f64, Expand);
-    setOperationAction(ISD::FFLOOR, MVT::v1f64, Expand);
-    setOperationAction(ISD::FMA, MVT::v1f64, Expand);
-    setOperationAction(ISD::FMUL, MVT::v1f64, Expand);
-    setOperationAction(ISD::FNEARBYINT, MVT::v1f64, Expand);
-    setOperationAction(ISD::FNEG, MVT::v1f64, Expand);
-    setOperationAction(ISD::FPOW, MVT::v1f64, Expand);
-    setOperationAction(ISD::FREM, MVT::v1f64, Expand);
-    setOperationAction(ISD::FROUND, MVT::v1f64, Expand);
-    setOperationAction(ISD::FROUNDEVEN, MVT::v1f64, Expand);
-    setOperationAction(ISD::FRINT, MVT::v1f64, Expand);
-    setOperationAction(ISD::FSIN, MVT::v1f64, Expand);
-    setOperationAction(ISD::FSINCOS, MVT::v1f64, Expand);
-    setOperationAction(ISD::FSQRT, MVT::v1f64, Expand);
-    setOperationAction(ISD::FSUB, MVT::v1f64, Expand);
-    setOperationAction(ISD::FTRUNC, MVT::v1f64, Expand);
-    setOperationAction(ISD::SETCC, MVT::v1f64, Expand);
-    setOperationAction(ISD::BR_CC, MVT::v1f64, Expand);
-    setOperationAction(ISD::SELECT, MVT::v1f64, Expand);
-    setOperationAction(ISD::SELECT_CC, MVT::v1f64, Expand);
-    setOperationAction(ISD::FP_EXTEND, MVT::v1f64, Expand);
-
-    setOperationAction(ISD::FP_TO_SINT, MVT::v1i64, Expand);
-    setOperationAction(ISD::FP_TO_UINT, MVT::v1i64, Expand);
-    setOperationAction(ISD::SINT_TO_FP, MVT::v1i64, Expand);
-    setOperationAction(ISD::UINT_TO_FP, MVT::v1i64, Expand);
-    setOperationAction(ISD::FP_ROUND, MVT::v1f64, Expand);
-
-    setOperationAction(ISD::FP_TO_SINT_SAT, MVT::v1i64, Expand);
-    setOperationAction(ISD::FP_TO_UINT_SAT, MVT::v1i64, Expand);
-
-    setOperationAction(ISD::MUL, MVT::v1i64, Expand);
+    for (auto Op :
+         {ISD::FABS, ISD::FADD, ISD::FCEIL, ISD::FCOPYSIGN, ISD::FCOS,
----------------
The formatting is apparently off here. I used to like the old code because you could do a search for ISD::FROUND and see all the setOperationAction(ISD::FROUND, VT, Legal) lines come up. Alas.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.td:4981
 let Predicates = [HasNEON] in {
-def : Pat<(f64 (sint_to_fp (i64 (fp_to_sint f64:$Rn)))),
+def : Pat<(f64 (any_sint_to_fp (i64 (any_fp_to_sint f64:$Rn)))),
           (SCVTFv1i64 (i64 (FCVTZSv1i64 f64:$Rn)))>;
----------------
Are optimizations like this desirable (or always valid?) for strict nodes? Same for the loads below. Do we have test cases for them?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.td:6279
 defm : SIMDFPIndexedTiedPatterns<"FMLS",
-           TriOpFrag<(fma node:$MHS, (fneg node:$RHS), node:$LHS)> >;
+           TriOpFrag<(any_fma node:$MHS, (fneg node:$RHS), node:$LHS)> >;
 defm : SIMDFPIndexedTiedPatterns<"FMLS",
----------------
Do these make a lot of sense, with a strict fma but a non-strict fneg?


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

https://reviews.llvm.org/D114946