[SDAG] Optimize unordered comparison in soft-float mode

[anadolskiy]

Summary

Current implementation handles unordered comparison poorly in soft-float mode.
Consider (a ULE b) which is a <= b. It is lowered to (ledf2(a, b) <= 0 || unorddf2(a, b) != 0) (in general). We can do better job by lowering it to (__gtdf2(a, b) <= 0).
Such replacement is true for other CMP's (ult, ugt, uge). In general, we just call same function as for ordered case but negate comparison against zero.

[anadolskiy]

Added tests

[ab]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

118

There was one newline too many, now there are two. Remove them both?

130

Capital g, and name sounds like a function. What about something like "ShouldInvertCC" ?

184

"Invert CC for unordered comparisons." ?

187–188

Indentation seems off, clang-format?

216

"true" -> "/*isInteger=*/true" ?

test/CodeGen/AArch64/arm64-fp128.ll

153

Is this just "cset w0, gt" ?

167

Can this comment be removed now? I guess your change lets us undo some other transform (which you should probably look into by the way)

170–171

CHECK-NEXT?

171

You should check IFFALSE/RET29 somewhere.

178

CHECK-NEXT?

182

CHECK-NEXT?

test/CodeGen/Thumb2/float-cmp.ll

84

Hmm, these might benefit from stricter CHECK/NONE, since I guess the result gets inverted?

test/CodeGen/X86/fp-cmp-sf.ll

1 ↗

(On Diff #28773)

Can you chmod this file as non-executable? Also, what about "fpcmp-soft-float" or "fpcmp-soft-fp"? "sf" isn't descriptive enough IMO.

127 ↗

(On Diff #28773)

Why optsize/readnone? For that matter, if you're not doing CHECK-NEXT, why nounwind?

[anadolskiy]

Thanks Ahmed for review! Fixed yours remarks

[anadolskiy]

Fixed format

[anadolskiy]

ping

test/CodeGen/AArch64/arm64-fp128.ll

167

This comment explains line 171: why we compare with GE, instead of LT (that should be in case of ordered LT)

178

No, there is ldp instruction in between

182

same here

test/CodeGen/Thumb2/float-cmp.ll

84

Sorry, I didn't understand that. Can you please explain?

test/CodeGen/X86/fp-cmp-sf.ll

1 ↗

(On Diff #28773)

Why chmod this as non-executable? Other tests don't do that.

[ab]

Thanks for the update! To me, the logic is sound. Nitpick replies below.

test/CodeGen/AArch64/arm64-fp128.ll

167

Hmm, that's not how I understood the comment.

Checking both uno/__unordtf2 and oge/__getf2 is indeed an 'unfortunate "optimization"'. We used to do something like:

IR:

%1 = fcmp olt ...
br i1 %1, label %iftrue, label %iffalse

SelectionDAG:

%1 = fcmp olt ...
%2 = xor i1 %1, 1
brcond i1 %2, label %iffalse
br label %iftrue

Optimized SelectionDAG:

%1 = fcmp uge ...
brcond i1 %2, label %iffalse
br label %iftrue

It's this optimization that the comment complains about, I think, because a softened uge used to need two libcalls (as opposed to 1 for olt).

Whereas I read:

  bl __lttf2
  cmp w0, #0
  b.ge <iffalse>
iftrue:

as a straightforward lowering for:

%1 = fcmp olt ...
%2 = xor i1 %1, 1
brcond i1 %2, label %iffalse
br label %iftrue

via (given that __lttf2 returns < 0 / -1 on "olt" only):

%1 = call @__lttf2
%2 = icmp lt i32 %1, 0
%3 = xor i1 %2, 1
brcond i1 %3, label %iffalse
br label %iftrue

itself simplified to:

%1 = call @__lttf2
%2 = icmp ge i32 %1, 0
brcond i1 %2, label %iffalse
br label %iftrue

In any case, not a big deal, but I might have missed something, so: am I making sense?

test/CodeGen/Thumb2/float-cmp.ll

84

The IR does an UGT comparison, but this patch checks for OLE. Since UGT == !OLE, I imagine there's some code right after the call that inverts r0 somehow.

I'm saying it might be a good idea to check (in this case, using "NONE:") that code as well.

test/CodeGen/X86/fp-cmp-sf.ll

1 ↗

(On Diff #28773)

I mean this test file (fp-cmp-sf.ll) is executable in your patch (see the svn property a few lines above on phabricator). Before committing, please change your local fp-cmp-sf.ll file to not be executable:

chmod -x test/CodeGen/X86/fp-cmp-sf.ll

You might also have to

svn propdel svn:executable test/CodeGen/X86/fp-cmp-sf.ll

if you svn add-ed it already.

[anadolskiy]

Made new tests non-executable

[anadolskiy]

Thanks for comments, Ahmed

test/CodeGen/AArch64/arm64-fp128.ll

167

Yes, exactly. But there's no XOR in given IR, so we must explain why GE. And I agree that <unfortunately "optimizes"> is now just "optimizes"

[ab]

OK, this LGTM, thanks! Do you need someone to commit it for you?

lib/CodeGen/SelectionDAG/TargetLowering.cpp

TargetLowering::makeLibCall(SelectionDAG &DAG,
   CLI.setDebugLoc(dl).setChain(DAG.getEntryNode())
     .setCallee(getLibcallCallingConv(LC), RetTy, Callee, std::move(Args), 0)
     .setNoReturn(doesNotReturn).setDiscardResult(!isReturnValueUsed)
     .setSExtResult(signExtend).setZExtResult(!signExtend);
   return LowerCallTo(CLI);
 }
 
 
+
 /// SoftenSetCCOperands - Soften the operands of a comparison.  This code is
 /// shared among BR_CC, SELECT_CC, and SETCC handlers.
 void TargetLowering::softenSetCCOperands(SelectionDAG &DAG, EVT VT,
                                          SDValue &NewLHS, SDValue &NewRHS,
                                          ISD::CondCode &CCCode,
                                          SDLoc dl) const {
   assert((VT == MVT::f32 || VT == MVT::f64 || VT == MVT::f128)
          && "Unsupported setcc type!");
 
   // Expand into one or more soft-fp libcall(s).
   RTLIB::Libcall LC1 = RTLIB::UNKNOWN_LIBCALL, LC2 = RTLIB::UNKNOWN_LIBCALL;
+  bool getInverseCC = false;
   switch (CCCode) {
   case ISD::SETEQ:
   case ISD::SETOEQ:
     LC1 = (VT == MVT::f32) ? RTLIB::OEQ_F32 :
           (VT == MVT::f64) ? RTLIB::OEQ_F64 : RTLIB::OEQ_F128;
     break;
   case ISD::SETNE:
   case ISD::SETUNE:
void TargetLowering::softenSetCCOperands(SelectionDAG &DAG, EVT VT,
   case ISD::SETUO:
     LC1 = (VT == MVT::f32) ? RTLIB::UO_F32 :
           (VT == MVT::f64) ? RTLIB::UO_F64 : RTLIB::UO_F128;
     break;
   case ISD::SETO:
     LC1 = (VT == MVT::f32) ? RTLIB::O_F32 :
           (VT == MVT::f64) ? RTLIB::O_F64 : RTLIB::O_F128;
     break;
-  default:
-    LC1 = (VT == MVT::f32) ? RTLIB::UO_F32 :
-          (VT == MVT::f64) ? RTLIB::UO_F64 : RTLIB::UO_F128;
-    switch (CCCode) {
-    case ISD::SETONE:
-      // SETONE = SETOLT | SETOGT
-      LC1 = (VT == MVT::f32) ? RTLIB::OLT_F32 :
-            (VT == MVT::f64) ? RTLIB::OLT_F64 : RTLIB::OLT_F128;
-      // Fallthrough
-    case ISD::SETUGT:
-      LC2 = (VT == MVT::f32) ? RTLIB::OGT_F32 :
-            (VT == MVT::f64) ? RTLIB::OGT_F64 : RTLIB::OGT_F128;
-      break;
-    case ISD::SETUGE:
-      LC2 = (VT == MVT::f32) ? RTLIB::OGE_F32 :
-            (VT == MVT::f64) ? RTLIB::OGE_F64 : RTLIB::OGE_F128;
-      break;
+  case ISD::SETUEQ:
+    LC1 = (VT == MVT::f32) ? RTLIB::UO_F32 :
+          (VT == MVT::f64) ? RTLIB::UO_F64 : RTLIB::UO_F128;
+    LC2 = (VT == MVT::f32) ? RTLIB::OEQ_F32 :
+          (VT == MVT::f64) ? RTLIB::OEQ_F64 : RTLIB::OEQ_F128;
+  default:
+    // Inverse CC for unordered comparison
+    getInverseCC = true;
+    switch (CCCode) {
     case ISD::SETULT:
-      LC2 = (VT == MVT::f32) ? RTLIB::OLT_F32 :
-            (VT == MVT::f64) ? RTLIB::OLT_F64 : RTLIB::OLT_F128;
-      break;
+    LC1 = (VT == MVT::f32) ? RTLIB::OGE_F32 :
+            (VT == MVT::f64) ? RTLIB::OGE_F64 : RTLIB::OGE_F128;
     case ISD::SETULE:
-      LC2 = (VT == MVT::f32) ? RTLIB::OLE_F32 :
+    LC1 = (VT == MVT::f32) ? RTLIB::OGT_F32 :
+            (VT == MVT::f64) ? RTLIB::OGT_F64 : RTLIB::OGT_F128;
+    break;
+    case ISD::SETUGT:
+    LC1 = (VT == MVT::f32) ? RTLIB::OLE_F32 :
             (VT == MVT::f64) ? RTLIB::OLE_F64 : RTLIB::OLE_F128;
-      break;
-    case ISD::SETUEQ:
-      LC2 = (VT == MVT::f32) ? RTLIB::OEQ_F32 :
-            (VT == MVT::f64) ? RTLIB::OEQ_F64 : RTLIB::OEQ_F128;
-      break;
+    case ISD::SETUGE:
+    LC1 = (VT == MVT::f32) ? RTLIB::OLT_F32 :
+            (VT == MVT::f64) ? RTLIB::OLT_F64 : RTLIB::OLT_F128;
     default: llvm_unreachable("Do not know how to soften this setcc!");
     }
   }
 
   // Use the target specific return value for comparions lib calls.
   EVT RetVT = getCmpLibcallReturnType();
   SDValue Ops[2] = { NewLHS, NewRHS };
   NewLHS = makeLibCall(DAG, LC1, RetVT, Ops, 2, false/*sign irrelevant*/,
-                       dl).first;
+             dl).first;
   NewRHS = DAG.getConstant(0, dl, RetVT);
+
   CCCode = getCmpLibcallCC(LC1);
+  if (getInverseCC)
+    CCCode = getSetCCInverse(CCCode, true);
+
   if (LC2 != RTLIB::UNKNOWN_LIBCALL) {
-    SDValue Tmp = DAG.getNode(ISD::SETCC, dl,
-                              getSetCCResultType(*DAG.getContext(), RetVT),
-                              NewLHS, NewRHS, DAG.getCondCode(CCCode));
-    NewLHS = makeLibCall(DAG, LC2, RetVT, Ops, 2, false/*sign irrelevant*/,
-                         dl).first;
-    NewLHS = DAG.getNode(ISD::SETCC, dl,
-                         getSetCCResultType(*DAG.getContext(), RetVT), NewLHS,
-                         NewRHS, DAG.getCondCode(getCmpLibcallCC(LC2)));
-    NewLHS = DAG.getNode(ISD::OR, dl, Tmp.getValueType(), Tmp, NewLHS);
-    NewRHS = SDValue();
+  SDValue Tmp = DAG.getNode(ISD::SETCC, dl,
+    getSetCCResultType(*DAG.getContext(), RetVT),
+    NewLHS, NewRHS, DAG.getCondCode(CCCode));
+  NewLHS = makeLibCall(DAG, LC2, RetVT, Ops, 2, false/*sign irrelevant*/,
+    dl).first;
+  NewLHS = DAG.getNode(ISD::SETCC, dl,
+    getSetCCResultType(*DAG.getContext(), RetVT), NewLHS,
+    NewRHS, DAG.getCondCode(getCmpLibcallCC(LC2)));
+  NewLHS = DAG.getNode(ISD::OR, dl, Tmp.getValueType(), Tmp, NewLHS);
+  NewRHS = SDValue();
   }
 }
 
 /// getJumpTableEncoding - Return the entry encoding for a jump table in the
 /// current function.  The returned value is a member of the
 /// MachineJumpTableInfo::JTEntryKind enum.
 unsigned TargetLowering::getJumpTableEncoding() const {
   // In non-pic modes, just use the address of a block.

test/CodeGen/AArch64/arm64-fp128.ll

 ; CHECK-LABEL: test_setcc2:
 
   %lhs = load fp128, fp128* @lhs, align 16
   %rhs = load fp128, fp128* @rhs, align 16
 ; CHECK: ldr q0, [{{x[0-9]+}}, :lo12:lhs]
 ; CHECK: ldr q1, [{{x[0-9]+}}, :lo12:rhs]
 
   %val = fcmp ugt fp128 %lhs, %rhs
-; CHECK: bl      __gttf2
+; CHECK: bl      __letf2
 ; CHECK: cmp     w0, #0
 ; CHECK: cset   [[GT:w[0-9]+]], gt
 
-; CHECK: bl      __unordtf2
-; CHECK: cmp     w0, #0
-; CHECK: cset   [[UNORDERED:w[0-9]+]], ne
-; CHECK: orr     w0, [[UNORDERED]], [[GT]]
-
   ret i1 %val
 ; CHECK: ret
 }
 
 define i32 @test_br_cc() {
 ; CHECK-LABEL: test_br_cc:
 
   %lhs = load fp128, fp128* @lhs, align 16
   %rhs = load fp128, fp128* @rhs, align 16
 ; CHECK: ldr q0, [{{x[0-9]+}}, :lo12:lhs]
 ; CHECK: ldr q1, [{{x[0-9]+}}, :lo12:rhs]
 
   ; olt == !uge, which LLVM unfortunately "optimizes" this to.
   %cond = fcmp olt fp128 %lhs, %rhs
-; CHECK: bl      __getf2
-; CHECK: cmp     w0, #0
-; CHECK: cset   [[OGE:w[0-9]+]], ge
-
-; CHECK: bl      __unordtf2
+; CHECK: bl      __lttf2
 ; CHECK: cmp     w0, #0
-; CHECK: cset   [[UNORDERED:w[0-9]+]], ne
-
-; CHECK: orr     [[UGE:w[0-9]+]], [[UNORDERED]], [[OGE]]
-; CHECK: cbnz [[UGE]], [[RET29:.LBB[0-9]+_[0-9]+]]
+; CHECK-NEXT: b.ge [[IFFALSE:.LBB[0-9]+_[0-9]+]]
   br i1 %cond, label %iftrue, label %iffalse
 
 iftrue:
   ret i32 42
 ; CHECK-NEXT: BB#
 ; CHECK-NEXT: movz w0, #0x2a
-; CHECK-NEXT: b [[REALRET:.LBB[0-9]+_[0-9]+]]
-
+; CHECK: ret
 iffalse:
   ret i32 29
-; CHECK: [[RET29]]:
-; CHECK-NEXT: movz w0, #0x1d
-; CHECK-NEXT: [[REALRET]]:
+; CHECK: movz w0, #0x1d
 ; CHECK: ret
 }
 
 define void @test_select(i1 %cond, fp128 %lhs, fp128 %rhs) {
 ; CHECK-LABEL: test_select:
 
   %val = select i1 %cond, fp128 %lhs, fp128 %rhs
   store fp128 %val, fp128* @lhs, align 16
 ; CHECK: tst w0, #0x1

test/CodeGen/Mips/mips16fpe.ll

;16hf-LABEL: test_ltsf2:
   %0 = load float, float* @x, align 4
   %1 = load float, float* @xx, align 4
   %lnot = fcmp uge float %0, %1
   %2 = load float, float* @y, align 4
   %cmp1 = fcmp olt float %0, %2
   %and2 = and i1 %lnot, %cmp1
   %and = zext i1 %and2 to i32
   store i32 %and, i32* @ltsf2_result, align 4
-;16hf:  lw  ${{[0-9]+}}, %call16(__mips16_unordsf2)(${{[0-9]+}})
+;16hf:  lw  ${{[0-9]+}}, %call16(__mips16_ltsf2)(${{[0-9]+}})
 ;16hf:  lw  ${{[0-9]+}}, %call16(__mips16_ltsf2)(${{[0-9]+}})
   ret void
 }
 
 define void @test_ltdf2() nounwind {
 entry:
 ;16hf-LABEL: test_ltdf2:
   %0 = load double, double* @xd, align 8
   %1 = load double, double* @xxd, align 8
   %lnot = fcmp uge double %0, %1
   %2 = load double, double* @yd, align 8
   %cmp1 = fcmp olt double %0, %2
   %and2 = and i1 %lnot, %cmp1
   %and = zext i1 %and2 to i32
   store i32 %and, i32* @ltdf2_result, align 4
-;16hf:  lw  ${{[0-9]+}}, %call16(__mips16_unorddf2)(${{[0-9]+}})
+;16hf:  lw  ${{[0-9]+}}, %call16(__mips16_ltdf2)(${{[0-9]+}})
 ;16hf:  lw  ${{[0-9]+}}, %call16(__mips16_ltdf2)(${{[0-9]+}})
   ret void
 }
 
 define void @test_lesf2() nounwind {
 entry:
 ;16hf-LABEL: test_lesf2:
   %0 = load float, float* @x, align 4

test/CodeGen/Thumb2/float-cmp.ll

 ; HARD: vcmpe.f32
 ; HARD: moveq r0, #1
 ; HARD: movvs r0, #1
   %1 = fcmp ueq float %a, %b
   ret i1 %1
 }
 define i1 @cmp_f_ugt(float %a, float %b) {
 ; CHECK-LABEL: cmp_f_ugt:
-; NONE: bl __aeabi_fcmpgt
-; NONE: bl __aeabi_fcmpun
+; NONE: bl __aeabi_fcmple
 ; HARD: vcmpe.f32
 ; HARD: movhi r0, #1
   %1 = fcmp ugt float %a, %b
   ret i1 %1
 }
 define i1 @cmp_f_uge(float %a, float %b) {
 ; CHECK-LABEL: cmp_f_uge:
-; NONE: bl __aeabi_fcmpge
-; NONE: bl __aeabi_fcmpun
+; NONE: bl __aeabi_fcmplt
 ; HARD: vcmpe.f32
 ; HARD: movpl r0, #1
   %1 = fcmp uge float %a, %b
   ret i1 %1
 }
 define i1 @cmp_f_ult(float %a, float %b) {
 ; CHECK-LABEL: cmp_f_ult:
-; NONE: bl __aeabi_fcmplt
-; NONE: bl __aeabi_fcmpun
+; NONE: bl __aeabi_fcmpge
 ; HARD: vcmpe.f32
 ; HARD: movlt r0, #1
   %1 = fcmp ult float %a, %b
   ret i1 %1
 }
 define i1 @cmp_f_ule(float %a, float %b) {
 ; CHECK-LABEL: cmp_f_ule:
-; NONE: bl __aeabi_fcmple
-; NONE: bl __aeabi_fcmpun
+; NONE: bl __aeabi_fcmpgt
 ; HARD: vcmpe.f32
 ; HARD: movle r0, #1
   %1 = fcmp ule float %a, %b
   ret i1 %1
 }
 define i1 @cmp_f_une(float %a, float %b) {
 ; CHECK-LABEL: cmp_f_une:
 ; NONE: bl __aeabi_fcmpeq

 ; SP: bl __aeabi_dcmpun
 ; DP: vcmpe.f64
 ; DP: movvc r0, #1
   %1 = fcmp ord double %a, %b
   ret i1 %1
 }
 define i1 @cmp_d_ugt(double %a, double %b) {
 ; CHECK-LABEL: cmp_d_ugt:
-; NONE: bl __aeabi_dcmpgt
-; NONE: bl __aeabi_dcmpun
-; SP: bl __aeabi_dcmpgt
-; SP: bl __aeabi_dcmpun
+; NONE: bl __aeabi_dcmple
+; SP: bl __aeabi_dcmple
 ; DP: vcmpe.f64
 ; DP: movhi r0, #1
   %1 = fcmp ugt double %a, %b
   ret i1 %1
 }
 
 define i1 @cmp_d_ult(double %a, double %b) {
 ; CHECK-LABEL: cmp_d_ult:
-; NONE: bl __aeabi_dcmplt
-; NONE: bl __aeabi_dcmpun
-; SP: bl __aeabi_dcmplt
-; SP: bl __aeabi_dcmpun
+; NONE: bl __aeabi_dcmpge
+; SP: bl __aeabi_dcmpge
 ; DP: vcmpe.f64
 ; DP: movlt r0, #1
   %1 = fcmp ult double %a, %b
   ret i1 %1
 }
 
 
 define i1 @cmp_d_uno(double %a, double %b) {

 ; DP: moveq r0, #1
 ; DP: movvs r0, #1
   %1 = fcmp ueq double %a, %b
   ret i1 %1
 }
 
 define i1 @cmp_d_uge(double %a, double %b) {
 ; CHECK-LABEL: cmp_d_uge:
-; NONE: bl __aeabi_dcmpge
-; NONE: bl __aeabi_dcmpun
-; SP: bl __aeabi_dcmpge
-; SP: bl __aeabi_dcmpun
+; NONE: bl __aeabi_dcmplt
+; SP: bl __aeabi_dcmplt
 ; DP: vcmpe.f64
 ; DP: movpl r0, #1
   %1 = fcmp uge double %a, %b
   ret i1 %1
 }
 
 define i1 @cmp_d_ule(double %a, double %b) {
 ; CHECK-LABEL: cmp_d_ule:
-; NONE: bl __aeabi_dcmple
-; NONE: bl __aeabi_dcmpun
-; SP: bl __aeabi_dcmple
-; SP: bl __aeabi_dcmpun
+; NONE: bl __aeabi_dcmpgt
+; SP: bl __aeabi_dcmpgt
 ; DP: vcmpe.f64
 ; DP: movle r0, #1
   %1 = fcmp ule double %a, %b
   ret i1 %1
 }
 
 define i1 @cmp_d_une(double %a, double %b) {
 ; CHECK-LABEL: cmp_d_une:
 ; NONE: bl __aeabi_dcmpeq
 ; SP: bl __aeabi_dcmpeq
 ; DP: vcmpe.f64
 ; DP: movne r0, #1
   %1 = fcmp une double %a, %b
   ret i1 %1
 }

test/CodeGen/Thumb2/float-intrinsics-double.ll

; HARD: vfma.f64
   ret double %1
 }
 
 ; FIXME: the FPv4-SP version is less efficient than the no-FPU version
 declare double     @llvm.fabs.f64(double %Val)
 define double @abs_d(double %a) {
 ; CHECK-LABEL: abs_d:
 ; NONE: bic r1, r1, #-2147483648
-; SP: bl __aeabi_dcmpgt
-; SP: bl __aeabi_dcmpun
 ; SP: bl __aeabi_dsub
+; SP: bl __aeabi_dcmple
 ; DP: vabs.f64 d0, d0
   %1 = call double @llvm.fabs.f64(double %a)
   ret double %1
 }
 
 declare double     @llvm.copysign.f64(double  %Mag, double  %Sgn)
 define double @copysign_d(double %a, double %b) {
 ; CHECK-LABEL: copysign_d: