[PATCH] D85744: [SVE] Lower fixed length FP minnum/maxnum

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3641
+  case ISD::FMAXNUM:
+    if (useSVEForFixedLengthVectorVT(Op.getValueType()))
+      return LowerToPredicatedOp(Op, DAG, AArch64ISD::FMAXNM_PRED);
----------------
I doubt you need the usesSVEFor... test or llvm_unreachable part.  I went a bit overboard for my original patch.  LowerToPredicatedOp has enough asserts to raise an alarm if something weird starts happening. 


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-fp-minmax.ll:558
+
+; NOTE: Check lines only cover the first VBYTES because the fmaxnm_v#f16 tests
+; already cover the general legalisation cases.
----------------
I'm happy with the tests, but just wanted to raise awareness of the "other" style of writing tests as shown by D85724.

Without going silly with covering all possible cases there's two options.  Stronger type legalisation tests as used by this patch and stronger "end result" testing as used by D85724.

Both are equally valid, it just comes down to whether you think type legalisation is already pretty much covered by other instruction tests and possible also which allows for better protection.

As I say, I'm not expecting any changes here, I'm just presenting the options.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85744