[PPC, FastISel] Fix ordered/unordered fcmp

[timshen]

Summary

Ordered comparison oge, ole and one cannot be implemented using a single instruction.

[timshen]

Updated comments.

[hfinkel]

I few comments on the comments, but otherwise, LGTM.

lib/Target/PowerPC/PPCFastISel.cpp

220

This wording is somewhat misleading. The 'bc' instruction only looks at one bit, but that bit selected via an b{pred} mnemonic is certainly never the 'un' bit. How about:

However, bc instruction only inspect the first 3 -> However, bc instruction only inspects one of the first 3 bits

221

to undesired -> to an undesired [note also spelling fix]

231

Remove "unexpectedly". [I understand why you say that, but the fact that the bits default to zero is not necessarily surprising, and I think you might confuse the reader of the comment by implying this is more subtle than it is]

[timshen]

Updated. Thanks!

lib/Target/PowerPC/PPCFastISel.cpp

CCAssignFn *PPCFastISel::usePPC32CCs(unsigned Flag) {
   else
     return RetCC_PPC;
 }
 
 static Optional<PPC::Predicate> getComparePred(CmpInst::Predicate Pred) {
   switch (Pred) {
     // These are not representable with any single compare.
     case CmpInst::FCMP_FALSE:
+    case CmpInst::FCMP_TRUE:
+    // Major concern about the following 6 cases is NaN result. The comparison
+    // result consists of 4 bits, indicating lt, eq, gt and un (unordered),
+    // only one of which will be set. The result is generated by fcmpu
+    // instruction. However, bc instruction only inspects one of the first 3
+    // bits, so when un is set, bc instruction may jump to to an undesired
+    // place.
+    //
+    // More specifically, if we expect an unordered comparison and un is set, we
+    // expect to always go to true branch; in such case UEQ, UGT and ULT still
+    // give false, which are undesired; but UNE, UGE, ULE happen to give true,
+    // since they are tested by inspecting !eq, !lt, !gt, respectively.
+    //
+    // Similarly, for ordered comparison, when un is set, we always expect the
+    // result to be false. In such case OGT, OLT and OEQ is good, since they are
+    // actually testing GT, LT, and EQ respectively, which are false. OGE, OLE
+    // and ONE are tested through !lt, !gt and !eq, and these are true.
     case CmpInst::FCMP_UEQ:
     case CmpInst::FCMP_UGT:
-    case CmpInst::FCMP_UGE:
     case CmpInst::FCMP_ULT:
-    case CmpInst::FCMP_ULE:
-    case CmpInst::FCMP_UNE:
-    case CmpInst::FCMP_TRUE:
+    case CmpInst::FCMP_OGE:
+    case CmpInst::FCMP_OLE:
+    case CmpInst::FCMP_ONE:
     default:
       return Optional<PPC::Predicate>();
 
     case CmpInst::FCMP_OEQ:
     case CmpInst::ICMP_EQ:
       return PPC::PRED_EQ;
 
     case CmpInst::FCMP_OGT:
     case CmpInst::ICMP_UGT:
     case CmpInst::ICMP_SGT:
       return PPC::PRED_GT;
 
-    case CmpInst::FCMP_OGE:
+    case CmpInst::FCMP_UGE:
     case CmpInst::ICMP_UGE:
     case CmpInst::ICMP_SGE:
       return PPC::PRED_GE;
 
     case CmpInst::FCMP_OLT:
     case CmpInst::ICMP_ULT:
     case CmpInst::ICMP_SLT:
       return PPC::PRED_LT;
 
-    case CmpInst::FCMP_OLE:
+    case CmpInst::FCMP_ULE:
     case CmpInst::ICMP_ULE:
     case CmpInst::ICMP_SLE:
       return PPC::PRED_LE;
 
-    case CmpInst::FCMP_ONE:
+    case CmpInst::FCMP_UNE:
     case CmpInst::ICMP_NE:
       return PPC::PRED_NE;
 
     case CmpInst::FCMP_ORD:
       return PPC::PRED_NU;
 
     case CmpInst::FCMP_UNO:
       return PPC::PRED_UN;

test/CodeGen/PowerPC/fast-isel-fcmp-nan.ll

+; RUN: llc -mtriple powerpc64le-unknown-linux-gnu -fast-isel -O0 < %s | FileCheck %s
+
+define i1 @TestULT(double %t0) {
+; CHECK-LABEL: TestULT:
+; CHECK: mcrf
+; CHECK: blr
+entry:
+  %t1 = fcmp ult double %t0, 0.000000e+00
+  br i1 %t1, label %good, label %bad
+
+bad:
+  ret i1 false
+
+good:
+  ret i1 true
+}
+
+define i1 @TestULE(double %t0) {
+; CHECK-LABEL: TestULE:
+; CHECK: fcmpu
+; CHECK-NEXT: ble
+; CHECK: blr
+entry:
+  %t1 = fcmp ule double %t0, 0.000000e+00
+  br i1 %t1, label %good, label %bad
+
+bad:
+  ret i1 false
+
+good:
+  ret i1 true
+}
+
+define i1 @TestUNE(double %t0) {
+; CHECK-LABEL: TestUNE:
+; CHECK: fcmpu
+; CHECK-NEXT: bne
+; CHECK: blr
+entry:
+  %t1 = fcmp une double %t0, 0.000000e+00
+  br i1 %t1, label %good, label %bad
+
+bad:
+  ret i1 false
+
+good:
+  ret i1 true
+}
+
+define i1 @TestUEQ(double %t0) {
+; CHECK-LABEL: TestUEQ:
+; CHECK: mcrf
+; CHECK: blr
+entry:
+  %t1 = fcmp ueq double %t0, 0.000000e+00
+  br i1 %t1, label %good, label %bad
+
+bad:
+  ret i1 false
+
+good:
+  ret i1 true
+}
+
+define i1 @TestUGT(double %t0) {
+; CHECK-LABEL: TestUGT:
+; CHECK: mcrf
+; CHECK: blr
+entry:
+  %t1 = fcmp ugt double %t0, 0.000000e+00
+  br i1 %t1, label %good, label %bad
+
+bad:
+  ret i1 false
+
+good:
+  ret i1 true
+}
+
+define i1 @TestUGE(double %t0) {
+; CHECK-LABEL: TestUGE:
+; CHECK: fcmpu
+; CHECK-NEXT: bge
+; CHECK: blr
+entry:
+  %t1 = fcmp uge double %t0, 0.000000e+00
+  br i1 %t1, label %good, label %bad
+
+bad:
+  ret i1 false
+
+good:
+  ret i1 true
+}
+
+define i1 @TestOLT(double %t0) {
+; CHECK-LABEL: TestOLT:
+; CHECK: fcmpu
+; CHECK-NEXT: blt
+; CHECK: blr
+entry:
+  %t1 = fcmp olt double %t0, 0.000000e+00
+  br i1 %t1, label %good, label %bad
+
+bad:
+  ret i1 false
+
+good:
+  ret i1 true
+}
+
+define i1 @TestOLE(double %t0) {
+; CHECK-LABEL: TestOLE:
+; CHECK: mcrf
+; CHECK: blr
+entry:
+  %t1 = fcmp ole double %t0, 0.000000e+00
+  br i1 %t1, label %good, label %bad
+
+bad:
+  ret i1 false
+
+good:
+  ret i1 true
+}
+
+define i1 @TestONE(double %t0) {
+; CHECK-LABEL: TestONE:
+; CHECK: mcrf
+; CHECK: blr
+entry:
+  %t1 = fcmp one double %t0, 0.000000e+00
+  br i1 %t1, label %good, label %bad
+
+bad:
+  ret i1 false
+
+good:
+  ret i1 true
+}
+
+define i1 @TestOEQ(double %t0) {
+; CHECK-LABEL: TestOEQ:
+; CHECK: fcmpu
+; CHECK-NEXT: beq
+; CHECK: blr
+entry:
+  %t1 = fcmp oeq double %t0, 0.000000e+00
+  br i1 %t1, label %good, label %bad
+
+bad:
+  ret i1 false
+
+good:
+  ret i1 true
+}
+
+define i1 @TestOGT(double %t0) {
+; CHECK-LABEL: TestOGT:
+; CHECK: fcmpu
+; CHECK-NEXT: bgt
+; CHECK: blr
+entry:
+  %t1 = fcmp ogt double %t0, 0.000000e+00
+  br i1 %t1, label %good, label %bad
+
+bad:
+  ret i1 false
+
+good:
+  ret i1 true
+}
+
+define i1 @TestOGE(double %t0) {
+; CHECK-LABEL: TestOGE:
+; CHECK: mcrf
+; CHECK: blr
+entry:
+  %t1 = fcmp oge double %t0, 0.000000e+00
+  br i1 %t1, label %good, label %bad
+
+bad:
+  ret i1 false
+
+good:
+  ret i1 true
+}