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

Tue Oct 22 16:55:35 PDT 2019

nemanjai marked 2 inline comments as done.
nemanjai added inline comments.

================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:550

+  if (TM.Options.UnsafeFPMath && Subtarget.hasVSX()) {
+    setOperationAction(ISD::FMAXNUM_IEEE, MVT::f64, Legal);
----------------
jsji wrote:
> 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.
When I originally did this, it would produce these nodes along with `ISD::FCANONICALIZE` when unsafe fp math isn't specified. However, the DAG combiner seems to have been modified to not do that any longer. The instructions themselves handle SNaNs correctly anyway so we can handle the inputs coming from `ISD::FCANONICALIZE` anyway.

However, I don't really see a point in legalizing `FMAXNUM/FMINNUM` since we will just get the `_IEEE` versions even with fast 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
----------------
jsji wrote:
> It would be great if we can pre-commit the testcase to show only difference .
I will add a RUN line without the FMF flags which will show the difference in codegen in the test case itself.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62993