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

[Jinsong Ji via Phabricator via llvm-commits]

jsji added a comment.

It is a great idea to exploit `xsmindp`/`xsmaxdp`! But looks like we make it more general than restricted to `UnsafeFPMath`?



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:550
 
+  if (TM.Options.UnsafeFPMath && Subtarget.hasVSX()) {
+    setOperationAction(ISD::FMAXNUM_IEEE, MVT::f64, Legal);
----------------
Why we need `TM.Options.UnsafeFPMath` here? 
If `ISD::FMAXNUM_IEEE` is generated, then the semantic is exact the same as `xsmaxdp, 
we should be safe to use `xsmaxdp`.

I think we can also add actions for `ISD::FMAXNUM`/`FMAXIMUM`  and `ISD::FMINNUM`/`FMINIMUM`, 
then we do need `TM.Options.UnsafeFPMath` or  `TM.Options.NoNansFPMath`/`NoSignedZerosFPMath` for them.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:984
+          (f64 (XSMINDP $A, $B))>;
+def : Pat<(f64 (fmaxnum_ieee f64:$A, f64:$B)),
+          (f64 (XSMAXDP $A, $B))>;
----------------
We can add similar patterns for `fmaxnum`/`fmaxinum` and `fminnum`/`fmininum` with `Predicates` ?


================
Comment at: llvm/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:   -mtriple=powerpc64le-unknown-unknown < %s | FileCheck %s
----------------
Maybe add `RUN` lines for  `--enable-no-nans-fp-math`/`-enable-no-signed-zeros-fp-math`?


================
Comment at: llvm/test/CodeGen/PowerPC/scalar-min-max.ll:7
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    xsmaxdp f1, f1, f2
+; CHECK-NEXT:    blr
----------------
It would be great if we can pre-commit the testcase to show only difference .


================
Comment at: llvm/test/CodeGen/PowerPC/scalar-min-max.ll:48
+
+attributes #0 = { "no-nans-fp-math"="true" "no-signed-zeros-fp-math"="true" }
----------------
Why we need these attributes?  Looks like these should be in different `RUN` line ?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62993