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

[Kit Barton via llvm-commits]

kbarton accepted this revision.
kbarton added a comment.
This revision is now accepted and ready to land.

With the exception of a few minor comments, this LGTM.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:247
@@ +246,3 @@
+  // The struct below is used by mayUseP9VSXScalarComparisonInstr to summarize
+  // information about the SELECT_CC node passed to it.
+  struct SelectCCSummary {
----------------
Could you please add a brief comment indicating what each field is meant to represent?

================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:474
@@ -460,1 +473,3 @@
 
+// This function looks for specific patterns that can be replaced by
+// ISA 3.0 instructions. There are two main pattern sets that we check for.
----------------
Please add doxygen-style comments, with a \brief and also the parameters and return values documented. 

================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:476
@@ +475,3 @@
+// ISA 3.0 instructions. There are two main pattern sets that we check for.
+// The first set is some vairations of
+//           select_cc t5, t6, t2, t4, setogt:ch
----------------
spelling: vairations -> variations

================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:484
@@ +483,3 @@
+//    t20: i1 = and t17, t19
+//  t23: f64 = select_cc t20, Constant:i1<0>, t4, t2, setne:ch
+// Here we also compare FPs to select from FPs, but the comparison is done
----------------
Indentation here is off. Is that intentional?

================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:494
@@ +493,3 @@
+    return false;
+
+
----------------
Extra blank line here. Please remove. 

================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:526
@@ +525,3 @@
+        Summary.Comp1 = N->getOperand(1);
+        break;
+      case ISD::CondCode::SETLE:
----------------
Replace this break with return true, unless there is something else that needs to be done before the return at the end of the function. 

================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:534
@@ +533,3 @@
+        Summary.Comp1 = N->getOperand(0);
+        break;
+      default: return false;
----------------
Replace break with return true. 

================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:568
@@ +567,3 @@
+         Summary.Comp1 = N->getOperand(0).getOperand(0).getOperand(0);
+         break;
+       default:
----------------
Replace break with return true. 

================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2886
@@ +2885,3 @@
+    if (PPCSubTarget->hasP9VSXScalarFPCompMinMax()) {
+      SelectCCSummary Summary;
+      if (mayUseP9VSXScalarComparisonInstr(N, Summary)) {
----------------
This is not initialized before passing to mayUseP9VSXScalarComparisonInstr. 
Is the assumption that all fields will be set inside the mayUseP9VSXScalarComparisonInstr?


http://reviews.llvm.org/D20019