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

[John Brawn via Phabricator via llvm-commits]

john.brawn planned changes to this revision.
john.brawn added a comment.

I'll split off the parts that are purely patterns into separate patches.



================
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,
----------------
dmgreen wrote:
> Was LLROUND of f16 previously Legal by default? So this doesn't alter anything by explicitly marking it?
Yes, TargetLoweringBase::initActions declares a bunch of operations as Expand for all floating-point types except f16 which makes them Legal by default (possibly a bug in TargetLoweringBase::initActions).


================
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,
----------------
dmgreen wrote:
> 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.
clang-format is a bit strange about how it likes to format these kinds of for loops, in that it has three different schemes (entries in list closely packed; entries in list vertical-aligned using whitespace; each entry on its own line) that it chooses between, and adding or removing elements can cause the whole thing to be dramatically reformatted. It wants here to use the "each entry on its own line" scheme which defeats the point of using a for loop (making the thing more compact so you can see the "these are the operations that are expanded for v1f64" part on one screen) so I've ignored it.


================
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)))>;
----------------
dmgreen wrote:
> Are optimizations like this desirable (or always valid?) for strict nodes? Same for the loads below. Do we have test cases for them?
Anything where there's a one-to-one mapping between a strict selectiondag node and a floating-point instruction is fine (or more generally where we have an instruction sequence that can cause the same floating-point exceptions in the same order and respects rounding modes).

These aren't tested, no. Actually it probably makes sense to move these kinds of patterns into separate patches and add tests for them there, as that can be done separately from the basic isel stuff.


================
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",
----------------
dmgreen wrote:
> Do these make a lot of sense, with a strict fma but a non-strict fneg?
fneg is purely a bit-flipping operation that doesn't have strict/non-strict versions and combining it doesn't change exception/rounding behaviour.


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

https://reviews.llvm.org/D114946