[PATCH] D20019: [PPC] exploitation of new xscmp*, as well as xsmaxcdp and xsmincdp

[Chuang-Yu Cheng via llvm-commits]

cycheng added a comment.

  Group1 Testcases:
  define {double|float} @{max|min}_test{1|2}{_float}(%x, %y) #1
  define {double|float} @{max|min}_test{1|2}{_float}_eq(%x, %y) #1
  define {double|float} @fast_{max|min}_test{1|2}{_float}(%x, %y) #2
  define {double|float} @fast_{max|min}_test{1|2}{_float}_eq(%x, %y) #2
  Total: 8*4 = 32
  
  Group2 Testcases:
  define {double|float} @fast_{double|float}_{ugt|ult|ogt|olt|uge|ule|oge|ole}(%x, %y, %a, %b) #1
  define {double|float} @nan_{double|float}_{ugt|ult|ogt|olt|uge|ule|oge|ole}(%x, %y, %a, %b) #2
  Total: 16*2 = 32
  
  Group3 Testcases:
  define double @{one|oeq}_test{_fast}(%x, %y) {#1|#2}
  Total: 4



- The prefix 'fast_' for functions is because of #1 {"no-nans-fp-math"="true"} or #2 {"no-nans-fp-math"="false"}? because the naming rule is a little bit different between Group1 and Group2.
- In Group2, how about unifying function naming when data type is double? I.e. omit "double" in function name when data type is double, as your 1st and 3rd test group naming rule.

Some other observations of test cases, just for reference

  test cases for this statement:
    } else if (N->getOperand(0).getValueType() == MVT::i1) {
      ..
    }
  
  define {float|double} @fast_{min|max}_test{1|2}{_float}_eq(%x, %y) #2
  define {float|double} @nan_{float|double}_{ugt|ult|oge|ole}(%x, %y, %a, %b) #2
  define double @one_test(double %x, double %y) #2


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:525
@@ +524,3 @@
+        return false;
+
+      ISD::CondCode CC1 = cast<CondCodeSDNode>
----------------
Looks like we want to handle this pattern:

```
t23: {f32|f64} = select_cc t20, Constant:i1<0>, t8, t6, setne:ch
  t20: i1 = and t17, t19
  t17: i1 = setcc t4, t2, setXXX:ch
  t19: i1 = setcc t4, t2, seto:ch
```

================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2834
@@ +2833,3 @@
+      SelectCCSummary Summary;
+      if (useP9VSXScalarComparisonInstr(N, Summary)) {
+        if (Summary.Leave) break; // Will be handled in tablegen.
----------------
I feel it is a little bit strange, if useP9VSXScalarComparisonInstr returns true, it should mean we can use p9vsx instructions for N, but actually even if the function returns true, we still have some cases that can't use p9vsx instructions.

Would it be better if:

```
if (useP9VSXScalarComparisonInstr(N, Summary)) {
  if (Summary.CC == ISD::CondCode::SETNE) { return ..; }
  if (Summary.Comp0 == Summary.Ret0 && ..) { return ..; }
  if (Summary.Comp0 == Summary.Ret1 && ..) { return ..; }
  if (CurDAG->getTarget().Options.NoNaNsFPMath) { return ..; }
  llvm_unreachable(..);
}
```

So useP9VSXScalarComparisonInstr might need additional arguments to help it judge if N is able to use p9vsx instructions.


================
Comment at: test/CodeGen/PowerPC/vsx-p9.ll:492
@@ +491,3 @@
+entry:
+  %cmp = fcmp fast ult double %y, %x
+  %b.a = select i1 %cmp, double %b, double %a
----------------
Do we need the 'fast' flag when we have "no-nans-fp-math"="true" attribute?

================
Comment at: test/CodeGen/PowerPC/vsx-p9.ll:537
@@ +536,3 @@
+entry:
+  %cmp = fcmp fast ugt double %y, %x
+  %b.a = select i1 %cmp, double %b, double %a
----------------
ugt -> ogt?

================
Comment at: test/CodeGen/PowerPC/vsx-p9.ll:658
@@ +657,3 @@
+entry:
+  %cmp = fcmp fast ugt float %y, %x
+  %b.a = select i1 %cmp, float %b, float %a
----------------
ugt -> ogt?

================
Comment at: test/CodeGen/PowerPC/vsx-p9.ll:778
@@ +777,3 @@
+entry:
+  %cmp = fcmp fast uge double %y, %x
+  %b.a = select i1 %cmp, double %b, double %a
----------------
uge -> oge?

================
Comment at: test/CodeGen/PowerPC/vsx-p9.ll:899
@@ +898,3 @@
+entry:
+  %cmp = fcmp fast uge float %y, %x
+  %b.a = select i1 %cmp, float %b, float %a
----------------
uge -> oge ?


http://reviews.llvm.org/D20019