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

Tue Feb 8 11:18:20 PST 2022

dmgreen added inline comments.

================
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,
----------------
john.brawn wrote:
> dmgreen wrote:
> > john.brawn wrote:
> > > 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.
> > I find the loop quite hard to read, to be honest. It is difficult to see which operations the loop is acting on without more clear structure. Maybe it's trying to do too much all at once? We should make sure all new code is clang-formatted. (I know there is still some old code that isn't formatted yet).
> > 
> > But I do acknowledge that I sometimes have opinions about these kind of things that are not shared with the rest of coders. Simpler is better in my opinion, and simpler isn't always smaller.
> > 
> > I think that if we add this, it will either be clang-formatted now or it will be formatting by someone in the future. So best to come up with something that looks OK when formatted. Does splitting the loop into strict and nonstrict help?
> I had a bit of a look at how clang-format is deciding how to format things here, and it turns out that the decision of how to format things in columns just tries a different numbers of columns, puts items in the order it sees them into columns, then discards any layout with over-long lines. So it won't try having one item span multiple columns, or break a line early when it would be over-long. How it decides between column layout and tightly packed I didn't manage to figure out.
> 
> Anyway, the upshot is that rearranging the items slightly means clang-format can successfully find a layout, so I'll be doing that.
OK that's good. Clang format can sure be funny at times. Thanks for doing that.

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1483
+
+  // FIXME: We could potentially make use of the vector comparison instructions
+  // for STRICT_FSETCC and STRICT_FSETCSS, but there's a number of
----------------
john.brawn wrote:
> dmgreen wrote:
> > FCmp are a pretty common operation. Is it not something we would want to support, without expanding?
> > 
> > Don't we have intrinsics that would expect to become a single vector operation?
> Yes, it's something we would want to support, hence the FIXME. And with regards to intrinsics, generating code that's correct is more important than using a single instruction.
In my experience, FIXME's are things people leave in the code because they have no plan of fixing them. Correct code is of course important, but people also have an expectation of a certain level of performance from the intrinsics.

================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.td:4038
+     TriOpFrag<(any_fma node:$LHS, (fneg node:$MHS), node:$RHS)> >;
 defm FNMADD : ThreeOperandFPData<1, 0, "fnmadd",
+     TriOpFrag<(fneg (any_fma node:$LHS, node:$MHS, node:$RHS))> >;
----------------
Is there a test for FNMADD (and FNMSUB)?  I don't see it in D118487 either.

================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.td:4230
           (FCVTLv8i16 V128:$Rn)>;
-def : Pat<(v2f64 (fpextend (v2f32 V64:$Rn))), (FCVTLv2i32 V64:$Rn)>;
+def : Pat<(v2f64 (any_fpextend (v2f32 V64:$Rn))), (FCVTLv2i32 V64:$Rn)>;

----------------
None of the test below seem to be Neon. Are they tested somewhere else?

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

https://reviews.llvm.org/D114946