[PATCH] D62993: [PowerPC] Emit scalar min/max instructions with unsafe fp math

[Jinsong Ji via Phabricator via llvm-commits]

jsji added a comment.

Some nit, the biggest question is with `line 7223`



================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1304
   case PPCISD::FSEL:            return "PPCISD::FSEL";
+  case PPCISD::XSMAXCDP:        return "PPCISD::XSMAXCPD";
+  case PPCISD::XSMINCDP:        return "PPCISD::XSMINCDP";
----------------
Typo? `XSMAXCDP` not `XSMAXCPD`.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7210
   SDNodeFlags Flags;
   Flags.setNoInfs(true);
   Flags.setNoNaNs(true);
----------------
Although this won't have problem right now, because we always return before using `Flags`.
I think it would be better to move this after the new code, to avoid a potential trap for future programmers.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7223
+    switch (CC) {
+    default: return Op;
+    case ISD::SETOGT:
----------------
We will lose some opportunities here, eg: with `-mcpu=pwr9 --enable-no-nans-fp-math --enable-no-signed-zeros-fp-math`?
We will catch new opportunities for max/min, but will give up lowering all the other CC?


================
Comment at: lib/Target/PowerPC/PPCInstrVSX.td:1290
+            (f64 (XSMINDP $A, $B))>;
+  def : Pat<(f64 (fminnum_ieee f64:$A, (fcanonicalize f64:$B))),
+            (f64 (XSMINDP $A, $B))>;
----------------
Nit: The pattern order here is not consistent with above: instead of having one min, one max, we start to put all min together, hen max.  It won't have problem, but would be better to read/check if we make it consistent.


================
Comment at: test/CodeGen/PowerPC/ctr-minmaxnum.ll:69
 ; QPX: mtctr
-; QPX-NOT: bl fminf
+; QPX-NOT: xsmindp
 ; QPX: blr
----------------
QPX should NOT be affected, so shouldn't change here?


================
Comment at: test/CodeGen/PowerPC/ctr-minmaxnum.ll:144
 ; QPX: mtctr
-; QPX-NOT: bl fmax
+; QPX-NOT: xsmaxdp
 ; QPX: blr
----------------
QPX , shouldn't change.


================
Comment at: test/CodeGen/PowerPC/scalar-min-max.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mcpu=pwr8 -ppc-asm-full-reg-names --enable-unsafe-fp-math \
+; RUN:   -verify-machineinstrs --enable-no-signed-zeros-fp-math \
----------------
I would be more interested to see `-mcpu=pwr9` with `--enable-no-nans-fp-math` instead of `-mcpu=pwr8`. :)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62993