Finally implement correct ordered comparisons for PPC, even though

the code generated is not wonderful.  This turns a miscompilation into
a code quality bug (noted in the ppc readme).  This fixes PR642, which
is over 2 years old (!).  Nate, please review this.


git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@45742 91177308-0d34-0410-b5e6-96231b3b80d8

lib/Target/PowerPC/PPCISelDAGToDAG.cpp

@@ -624,29 +624,34 @@ static PPC::Predicate getPredicateForSetCC(ISD::CondCode CC) {
 /// getCRIdxForSetCC - Return the index of the condition register field
 /// associated with the SetCC condition, and whether or not the field is
 /// treated as inverted.  That is, lt = 0; ge = 0 inverted.
-static unsigned getCRIdxForSetCC(ISD::CondCode CC, bool& Inv) {
+///
+/// If this returns with Other != -1, then the returned comparison is an or of
+/// two simpler comparisons.  In this case, Invert is guaranteed to be false.
+static unsigned getCRIdxForSetCC(ISD::CondCode CC, bool &Invert, int &Other) {
+  Invert = false;
+  Other = -1;
   switch (CC) {
   default: assert(0 && "Unknown condition!"); abort();
-  case ISD::SETOLT:  // FIXME: This is incorrect see PR642.
+  case ISD::SETOLT:
-  case ISD::SETULT:
+  case ISD::SETLT:  return 0;                  // Bit #0 = SETOLT
-  case ISD::SETLT:  Inv = false;  return 0;
+  case ISD::SETOGT:
-  case ISD::SETOGE:  // FIXME: This is incorrect see PR642.
+  case ISD::SETGT:  return 1;                  // Bit #1 = SETOGT
+  case ISD::SETOEQ:
+  case ISD::SETEQ:  return 2;                  // Bit #2 = SETOEQ
+  case ISD::SETUO:  return 3;                  // Bit #3 = SETUO
   case ISD::SETUGE:
-  case ISD::SETGE:  Inv = true;   return 0;
+  case ISD::SETGE:  Invert = true; return 0;   // !Bit #0 = SETUGE
-  case ISD::SETOGT:  // FIXME: This is incorrect see PR642.
-  case ISD::SETUGT:
-  case ISD::SETGT:  Inv = false;  return 1;
-  case ISD::SETOLE:  // FIXME: This is incorrect see PR642.
   case ISD::SETULE:
-  case ISD::SETLE:  Inv = true;   return 1;
+  case ISD::SETLE:  Invert = true; return 1;   // !Bit #1 = SETULE
-  case ISD::SETOEQ:  // FIXME: This is incorrect see PR642.
-  case ISD::SETUEQ:
-  case ISD::SETEQ:  Inv = false;  return 2;
-  case ISD::SETONE:  // FIXME: This is incorrect see PR642.
   case ISD::SETUNE:
-  case ISD::SETNE:  Inv = true;   return 2;
+  case ISD::SETNE:  Invert = true; return 2;   // !Bit #2 = SETUNE
-  case ISD::SETO:   Inv = true;   return 3;
+  case ISD::SETO:   Invert = true; return 3;   // !Bit #3 = SETO
-  case ISD::SETUO:  Inv = false;  return 3;
+  case ISD::SETULT: Other = 0; return 3;       // SETOLT | SETUO
+  case ISD::SETUGT: Other = 1; return 3;       // SETOGT | SETUO
+  case ISD::SETUEQ: Other = 2; return 3;       // SETOEQ | SETUO
+  case ISD::SETOGE: Other = 1; return 2;       // SETOGT | SETOEQ
+  case ISD::SETOLE: Other = 0; return 2;       // SETOLT | SETOEQ
+  case ISD::SETONE: Other = 0; return 1;       // SETOLT | SETOGT
   }
   return 0;
 }
@@ -726,7 +731,8 @@ SDNode *PPCDAGToDAGISel::SelectSETCC(SDOperand Op) {
   }
   
   bool Inv;
-  unsigned Idx = getCRIdxForSetCC(CC, Inv);
+  int OtherCondIdx;
+  unsigned Idx = getCRIdxForSetCC(CC, Inv, OtherCondIdx);
   SDOperand CCReg = SelectCC(N->getOperand(0), N->getOperand(1), CC);
   SDOperand IntCR;
   
@@ -737,7 +743,7 @@ SDNode *PPCDAGToDAGISel::SelectSETCC(SDOperand Op) {
   CCReg = CurDAG->getCopyToReg(CurDAG->getEntryNode(), CR7Reg, CCReg, 
                                InFlag).getValue(1);
   
-  if (PPCSubTarget.isGigaProcessor())
+  if (PPCSubTarget.isGigaProcessor() && OtherCondIdx == -1)
     IntCR = SDOperand(CurDAG->getTargetNode(PPC::MFOCRF, MVT::i32, CR7Reg,
                                             CCReg), 0);
   else
@@ -745,13 +751,26 @@ SDNode *PPCDAGToDAGISel::SelectSETCC(SDOperand Op) {
   
   SDOperand Ops[] = { IntCR, getI32Imm((32-(3-Idx)) & 31),
                       getI32Imm(31), getI32Imm(31) };
-  if (!Inv) {
+  if (OtherCondIdx == -1 && !Inv)
     return CurDAG->SelectNodeTo(N, PPC::RLWINM, MVT::i32, Ops, 4);
-  } else {
+
-    SDOperand Tmp =
+  // Get the specified bit.
-      SDOperand(CurDAG->getTargetNode(PPC::RLWINM, MVT::i32, Ops, 4), 0);
+  SDOperand Tmp =
+    SDOperand(CurDAG->getTargetNode(PPC::RLWINM, MVT::i32, Ops, 4), 0);
+  if (Inv) {
+    assert(OtherCondIdx == -1 && "Can't have split plus negation");
     return CurDAG->SelectNodeTo(N, PPC::XORI, MVT::i32, Tmp, getI32Imm(1));
   }
+
+  // Otherwise, we have to turn an operation like SETONE -> SETOLT | SETOGT.
+  // We already got the bit for the first part of the comparison (e.g. SETULE).
+
+  // Get the other bit of the comparison.
+  Ops[1] = getI32Imm((32-(3-OtherCondIdx)) & 31);
+  SDOperand OtherCond = 
+    SDOperand(CurDAG->getTargetNode(PPC::RLWINM, MVT::i32, Ops, 4), 0);
+
+  return CurDAG->SelectNodeTo(N, PPC::OR, MVT::i32, Tmp, OtherCond);
 }
 
 

lib/Target/PowerPC/README.txt

@@ -706,3 +706,32 @@ __Z11no_overflowjj:
 
 //===---------------------------------------------------------------------===//
 
+We compile some FP comparisons into an mfcr with two rlwinms and an or.  For
+example:
+#include <math.h>
+int test(double x, double y) { return islessequal(x, y);}
+int test2(double x, double y) {  return islessgreater(x, y);}
+int test3(double x, double y) {  return !islessequal(x, y);}
+
+Compiles into (all three are similar, but the bits differ):
+
+_test:
+	fcmpu cr7, f1, f2
+	mfcr r2
+	rlwinm r3, r2, 29, 31, 31
+	rlwinm r2, r2, 31, 31, 31
+	or r3, r2, r3
+	blr 
+
+GCC compiles this into:
+
+ _test:
+	fcmpu cr7,f1,f2
+	cror 30,28,30
+	mfcr r3
+	rlwinm r3,r3,31,1
+	blr
+        
+which is more efficient and can use mfocr.  See PR642 for some more context.
+
+//===---------------------------------------------------------------------===//

test/CodeGen/PowerPC/compare-fcmp-ord.ll

@@ -0,0 +1,27 @@
+; RUN: llvm-as < %s | llc -march=ppc32 | grep or | count 3
+; This should produce one 'or' or 'cror' instruction per function.
+
+target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:64:64-v128:128:128-a0:0:64-f80:128:128"
+target triple = "i686-apple-darwin8"
+
+define i32 @test(double %x, double %y) nounwind  {
+entry:
+	%tmp3 = fcmp ole double %x, %y		; <i1> [#uses=1]
+	%tmp345 = zext i1 %tmp3 to i32		; <i32> [#uses=1]
+	ret i32 %tmp345
+}
+
+define i32 @test2(double %x, double %y) nounwind  {
+entry:
+	%tmp3 = fcmp one double %x, %y		; <i1> [#uses=1]
+	%tmp345 = zext i1 %tmp3 to i32		; <i32> [#uses=1]
+	ret i32 %tmp345
+}
+
+define i32 @test3(double %x, double %y) nounwind  {
+entry:
+	%tmp3 = fcmp ugt double %x, %y		; <i1> [#uses=1]
+	%tmp34 = zext i1 %tmp3 to i32		; <i32> [#uses=1]
+	ret i32 %tmp34
+}
+