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

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:555
+    setOperationAction(ISD::FMINNUM_IEEE, MVT::f64, Legal);
+    setOperationAction(ISD::FMINNUM_IEEE, MVT::f32, Legal);
+  }
----------------
steven.zhang wrote:
> We will get the ISD::FMAXNUM/ISD::FMINNUM node if mark it as legal.
> ```
> define dso_local float @testfmax_fast(float %a, float %b) {
> entry:
>   %cmp = fcmp fast ogt float %a, %b
>   %cond = select i1 %cmp, float %a, float %b
>   ret float %cond
> }
> ```
> llc test.ll -mattr=+vsx
> 
> And also for the intrinsic llvm.minnum/llvm.maxnum. 
> ```
> Initial selection DAG: %bb.0 'testfmax_fast:entry'
> SelectionDAG has 11 nodes:
>   t0: ch = EntryToken
>   t2: f32,ch = CopyFromReg t0, Register:f32 %0
>   t4: f32,ch = CopyFromReg t0, Register:f32 %1
>   t6: i1 = setcc nnan ninf nsz arcp contract afn reassoc t2, t4, setgt:ch
>     t7: f32 = fmaxnum t2, t4
>   t9: ch,glue = CopyToReg t0, Register:f32 $f1, t7
>   t10: ch = PPCISD::RET_FLAG t9, Register:f32 $f1, t9:1
> ```
> The node is built directly not combined by select_cc. And I think, we need to lower it if we know that, the operand is NaN or not(i.e. isKnownNeverNaN()).
Hmm, ignore the above comments.  It is right to have DAG generated the IEEE node instead of the non-IEEE, as the hw has instruction semantics equal.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7225
+    case ISD::SETOGT:
+      return DAG.getNode(PPCISD::XSMAXCDP, dl, Op.getValueType(), LHS, RHS);
+    case ISD::SETOLT:
----------------
steven.zhang wrote:
> Do we need logic to handle the case that if the op is NaN ?
> ```
> If src1 or src2 is a SNaN, an Invalid Operation
> exception occurs.
> If either src1 or src2 is a NaN, result is src2.
> Otherwise, if src1 is less than src2, result is src1.
> Otherwise, result is src2.
> ```
> 
> The ISA documentation is a bit confusing here. Isn't NaN including SNaN and QNaN ? The condition in the second if cover the first one.
Please also ignore above comments as after double confirm, the XSMAXCDP/XSMINCDP perfectly match the semantics, no matter if the operand is NaN or not.


================
Comment at: test/CodeGen/PowerPC/scalar-min-max.ll:12
+; RUN:   --check-prefix=NO-FAST-P8
+define dso_local float @testfmax(float %a, float %b) local_unnamed_addr #0 {
+; CHECK-LABEL: testfmax:
----------------
It would be great if we have some test to verify the behavior if operand is SNaN/QNaN for P9. However, this is NOT a must.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62993