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

[John Brawn via Phabricator via llvm-commits]

john.brawn added a comment.

> Is a f128 fminimum / fmaximum something that should be handled? I don't see any tests for fminimum in general.

Yes, it should, I'll do that and add tests.



================
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
----------------
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.


================
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:
> 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.


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

https://reviews.llvm.org/D114946