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

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang requested changes to this revision.
steven.zhang added a comment.
This revision now requires changes to proceed.

I see there is one sanity failed.

  FAIL: LLVM :: CodeGen/PowerPC/ctr-minmaxnum.ll (29517 of 50289)
  ******************** TEST 'LLVM :: CodeGen/PowerPC/ctr-minmaxnum.ll' FAILED ********************
  Script:
  --
  : 'RUN: at line 1';   /home/qshanz/work/build/bin/llc -mtriple=powerpc64-unknown-linux-gnu -verify-machineinstrs -mcpu=pwr7 < /home/qshanz/work/llvm/llvm/test/CodeGen/PowerPC/ctr-minmaxnum.ll | /home/qshanz/work/build/bin/FileCheck /home/qshanz/work/llvm/llvm/test/CodeGen/PowerPC/ctr-minmaxnum.ll
  : 'RUN: at line 2';   /home/qshanz/work/build/bin/llc -mtriple=powerpc64-unknown-linux-gnu -verify-machineinstrs -mcpu=a2q < /home/qshanz/work/llvm/llvm/test/CodeGen/PowerPC/ctr-minmaxnum.ll | /home/qshanz/work/build/bin/FileCheck /home/qshanz/work/llvm/llvm/test/CodeGen/PowerPC/ctr-minmaxnum.ll --check-prefix=QPX
  --
  Exit Code: 2
  
  Command Output (stderr):
  --
  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



================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:555
+    setOperationAction(ISD::FMINNUM_IEEE, MVT::f64, Legal);
+    setOperationAction(ISD::FMINNUM_IEEE, MVT::f32, Legal);
+  }
----------------
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()).


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7221
 
+  if (Subtarget.hasP9Vector() && LHS == TV && RHS == FV) {
+    switch (CC) {
----------------
Is it hasVSX() more clear ?


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7225
+    case ISD::SETOGT:
+      return DAG.getNode(PPCISD::XSMAXCDP, dl, Op.getValueType(), LHS, RHS);
+    case ISD::SETOLT:
----------------
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.


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


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62993