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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai marked 5 inline comments as done.
nemanjai added a comment.

In D62993#1718495 <https://reviews.llvm.org/D62993#1718495>, @steven.zhang wrote:

> I see there is one sanity failed. 
>  LLVM ERROR: Cannot select: t14: f32 = fcanonicalize t2
>
>   t2: f32,ch = CopyFromReg t0, Register:f32 %2
>     t1: f32 = Register %2
>
> In function: test1
>  FileCheck error: '-' is empty.
>  FileCheck command line:  /home/qshanz/work/build/bin/FileCheck /home/qshanz/work/llvm/llvm/test/CodeGen/PowerPC/ctr-minmaxnum.ll


Ah, that's where I saw the issue I mentioned in https://reviews.llvm.org/D62993?id=203487#inline-622927
I'll fix it up in the next update.



================
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:
> 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.
I don't dispute that we will get the nodes if we mark them legal. However, I do not think that we will get these nodes in more situations than we get the `_IEEE` versions.

The way I see it, with `ninf nsz nnan`, the nodes are equivalent since the only difference between them is the handling of NaNs and (presumably) signed zeros.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7205
+  // presence of infinities.
+  if (!Subtarget.isISA3_0() && (!DAG.getTarget().Options.NoInfsFPMath ||
+                                !DAG.getTarget().Options.NoNaNsFPMath))
----------------
This needs to change from `isISA3_0()` to `hasP9Vector()`.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7221
 
+  if (Subtarget.hasP9Vector() && LHS == TV && RHS == FV) {
+    switch (CC) {
----------------
steven.zhang wrote:
> Is it hasVSX() more clear ?
I dont' understand this comment. This is only available in ISA3.0, so `hasVSX()` is not adequate. And VSX is a requirement for P9Vector, so why would I need both?


================
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.
Quiet NaNs are fine. Signaling NaNs cause exceptions. This is fine, signaling NaNs are supposed to cause exceptions as far as I can tell.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.td:120
 
+def SDT_FPMinMax : SDTypeProfile<1, 2, [
+  SDTCisSameAs<0, 1>, SDTCisSameAs<0, 2>, SDTCisFP<0>
----------------
steven.zhang wrote:
> SDT_PPCFPMinMax ?
Sounds good. Will do.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62993