Generate aeabi_cdcmple libcalls

[SjoerdMeijer]

Summary

ARM code size optimisation: rewrite aeabi_dcmplt and aeabi_dcmpgt (and also the less/greater equal variants) to __aeabi_cdcmple and _aeabi_cdrcmple, which avoids pulling in more helper functions from the math library during link time, and also cdcmple and cdrcmple are one and the same function (the latter just swaps the operands).

[compnerd]

lib/Target/ARM/ARMISelLowering.cpp

11460

This really feels wrong. This really should be done earlier in the SDAG. We could introduce a target specific SDNode for the operation.

11466

Shouldnt this also check for AAPCS? The routines are written with AAPCS in mind.

test/CodeGen/ARM/cdcmp.ll

2

Please add a test for Windows on ARM and ensure that the conversion is not done.

[SjoerdMeijer]

Thanks for reviewing! And I do share your concerns a bit. I have explored doing this transformation earlier, but I was struggling with this as the softening of the float operands and generation of libcalls is done really early during type legalization. The problems is that this cdcmple EABI call is different, it returns its results in the status flags and then all is needed is a conditional branch on that (no cmp instruction needed). Trying to shoehorn and hook this all in type legalization also feels wrong and very fiddly (but I am new to isel). So the elegance of this patch and fixup is that it is really small, and yes, the less pretty part is that this is a fix up. Again, I am new to this bit of llvm, so if you have more tips/suggestions how to solve this with a "target specific SDNode", then they are more than welcome.
Cheers,
Sjoerd.

[SjoerdMeijer]

I've added a check for AAPCS and also a Windows test case.

[EricWF]

Resigning as reviewer because I'm not qualified to review this.

[rengolin]

This is totally in the wrong place. Not only it's a hack on another combine, it's too late into the process.

For similar ABI handling, we have the constructor in ISelLowering to choose the right lib call. Look at divmod for the "correct way" (tm) to massage arguments.

It may not be pretty, but it also doesn't have the two critical problems this approach has:

1. We select functions and library calls, only to re-select them later.
2. It selectively passes on another combine, meaning there's no guarantee that all cases will be dealt with and validated.

I believe this works by luck and not design. That's why the more ugly library handling before DAGISel is executed is the only way.

cheers,
--renato

[SjoerdMeijer]

Hi Renato, thanks for the review and suggestions; I will be looking into it.
Cheers,
Sjoerd.

lib/Target/ARM/ARMISelLowering.cpp

for (unsigned BitInY = 0, NumActiveBits = OrCI.getActiveBits();
     V = DAG.getNode(ARMISD::BFI, dl, VT, V, X,
                     // Confusingly, the operand is an *inverted* mask.
                     DAG.getConstant(~Mask, dl, VT));
   }
 
   return V;
 }
 
+// Code size optimisation: rewrite __aeabi_dcmplt and __aeabi_dcmpgt (and
+// also the less/greater equal variants) to __aeabi_cdcmple and
+// __aeabi_cdrcmple, which avoids pulling in more helper functions from the
+// math library during link time, and also cdcmple and cdrcmple are one and the
+// same function but the latter just swaps the operands.  The approach taken
+// here is to fix up the isel dag after the initial lowering and softening to
+// calls dcmplt and dcmpgt. The reasons is that the libcalls are generated
+// during type legalization which is early in isel and the __aeabi_cdcmple
+// functions are different than the other EABI libcalls, i.e. they don't have a
+// return value but just update the status flags and this fixup is less
+// intrusive.
+static bool rewriteEABICmpLibcalls(SDNode *N, SelectionDAG &DAG) {
+  SDValue Cmp = N->getOperand(4);
+  SDValue CopyFromReg = Cmp->getOperand(0);
+
+  if (CopyFromReg.getOpcode() != ISD::CopyFromReg)
+    return false;
+
+  SDValue CSEnd = CopyFromReg->getOperand(0);
+  if (CSEnd.getOpcode() != ISD::CALLSEQ_END)
+    return false;
+
+  SDValue Call = CSEnd.getOperand(0);
+  if (Call.getOpcode() != ARMISD::CALL)
+    return false;
+
+  SDValue Symbol = Call.getOperand(1);
+  if (Symbol.getOpcode() != ISD::TargetExternalSymbol)
+    return false;
+
+  SDValue NewSym;
+  ExternalSymbolSDNode *ESN = cast<ExternalSymbolSDNode>(Symbol);
+  ARMCC::CondCodes CC;
+
+  StringRef SymbolName = ESN->getSymbol();
+  StringRef NewSymbolName;
+
+  if(SymbolName == "__aeabi_dcmpgt") {
+    NewSymbolName = "__aeabi_cdrcmple";
+    CC = ARMCC::CondCodes::HS;
+  } else if(SymbolName == "__aeabi_dcmplt") {
+    NewSymbolName = "__aeabi_cdcmple";
+    CC = ARMCC::CondCodes::HS;
+  } else if(SymbolName == "__aeabi_dcmple") {
+    NewSymbolName = "__aeabi_cdcmple";
+    CC = ARMCC::CondCodes::HI;
+  } else if(SymbolName == "__aeabi_dcmpge") {
+    NewSymbolName = "__aeabi_cdrcmple";
+    CC = ARMCC::CondCodes::HI;
+  } else
+    return false;
+
+  // Because we are going to remove the CMP node from the DAG and replace
+  // the uses with the glue code, we are making sure there is only 1 use.
+  if (!Cmp.hasOneUse())
+    return false;
+
+  NewSym = DAG.getTargetExternalSymbol(NewSymbolName.begin(), MVT::isVoid);
+
+  // A libcall has 8 operands, we keep all operands here, and only change
+  // the Symbol operand. As we are creating a new node that does not exist,
+  // function UpdateNodeOperands should mutate the operands in place.
+  ArrayRef< SDValue > Ops = { Call.getOperand(0), NewSym, Call.getOperand(2),
+                              Call.getOperand(3), Call.getOperand(4),
+                              Call.getOperand(5), Call.getOperand(6),
+                              Call.getOperand(7)};
+  DAG.UpdateNodeOperands(Call.getNode(), Ops);
+
+  DAG.ReplaceAllUsesWith(Cmp, SDValue(Cmp.getOperand(0).getNode(), 2));
+
+  // From the Run-time ABI for the ARM Architecture:
+  // "The 3-way comparison functions c*cmple, c*cmpeq and c*rcmple return their
+  // results in the CPSR Z and C flags. C is clear only if the operands are
+  // ordered and the first operand is less than the second. Z is set only when
+  // the operands are ordered and equal."
+  //
+  // The conditional branch, the BRCOND node, branches to false case, so therefore
+  // for the GT and LT case, we have condition code HS which tests if C==1, and
+  // for the GE and LE cases, we use Hi because that tests (C==1) && (Z==0).
+  SDValue ARMcc = DAG.getConstant(CC, SDLoc(N), MVT::i32);
+  DAG.ReplaceAllUsesWith(N->getOperand(2), ARMcc);
+  DAG.RemoveDeadNode(Cmp.getNode());
+  return true;
+}
+
 /// PerformBRCONDCombine - Target-specific DAG combining for ARMISD::BRCOND.
 SDValue
 ARMTargetLowering::PerformBRCONDCombine(SDNode *N, SelectionDAG &DAG) const {
+  if (getSubtarget()->isTargetAEABI() && getSubtarget()->isAAPCS_ABI() &&
+      rewriteEABICmpLibcalls(N, DAG))
+    return SDValue();
+
   SDValue Cmp = N->getOperand(4);
   if (Cmp.getOpcode() != ARMISD::CMPZ)
     // Only looking at NE cases.
     return SDValue();
 
   EVT VT = N->getValueType(0);
   SDLoc dl(N);
   SDValue LHS = Cmp.getOperand(0);

test/CodeGen/ARM/cdcmp.ll

+; RUN: llc -mcpu=cortex-m3 -mtriple=thumb-none-eabi %s -o - | FileCheck %s
+; RUN: llc -mtriple=thumb-windows %s -o - | FileCheck %s -check-prefix=WINDOWS
+
+define i32 @OLT() {
+entry:
+; CHECK-LABEL: OLT:
+; CHECK: __aeabi_cdcmple
+; WINDOWS-NOT: __aeabi_cdcmple
+  %retval = alloca i32, align 4
+  %A = alloca double, align 8
+  %B = alloca double, align 8
+  store double 1.000000e+00, double* %A, align 8
+  store double 1.000000e+01, double* %B, align 8
+  %0 = load double, double* %A, align 8
+  %1 = load double, double* %B, align 8
+  %cmp = fcmp olt double %0, %1
+  br i1 %cmp, label %if.then, label %if.end
+
+if.then:
+  store i32 0, i32* %retval, align 4
+  br label %return
+
+if.end:
+  store i32 1, i32* %retval, align 4
+  br label %return
+
+return:
+  %2 = load i32, i32* %retval, align 4
+  ret i32 %2
+}
+
+define i32 @OLE() {
+entry:
+; CHECK-LABEL: OLE:
+; CHECK: __aeabi_cdcmple
+; WINDOWS-NOT: __aeabi_cdcmple
+  %retval = alloca i32, align 4
+  %A = alloca double, align 8
+  %B = alloca double, align 8
+  store double 1.000000e+00, double* %A, align 8
+  store double 1.000000e+01, double* %B, align 8
+  %0 = load double, double* %A, align 8
+  %1 = load double, double* %B, align 8
+  %cmp = fcmp ole double %0, %1
+  br i1 %cmp, label %if.then, label %if.end
+
+if.then:
+  store i32 0, i32* %retval, align 4
+  br label %return
+
+if.end:
+  store i32 1, i32* %retval, align 4
+  br label %return
+
+return:
+  %2 = load i32, i32* %retval, align 4
+  ret i32 %2
+}
+
+define i32 @OGT() {
+entry:
+; CHECK-LABEL: OGT:
+; CHECK: __aeabi_cdrcmple
+; WINDOWS-NOT: __aeabi_cdrcmple
+  %retval = alloca i32, align 4
+  %A = alloca double, align 8
+  %B = alloca double, align 8
+  store double 1.000000e+00, double* %A, align 8
+  store double 1.000000e+01, double* %B, align 8
+  %0 = load double, double* %A, align 8
+  %1 = load double, double* %B, align 8
+  %cmp = fcmp ogt double %0, %1
+  br i1 %cmp, label %if.then, label %if.end
+
+if.then:
+  store i32 0, i32* %retval, align 4
+  br label %return
+
+if.end:
+  store i32 1, i32* %retval, align 4
+  br label %return
+
+return:
+  %2 = load i32, i32* %retval, align 4
+  ret i32 %2
+}
+
+define i32 @OGE() {
+entry:
+; CHECK-LABEL: OGE:
+; CHECK: __aeabi_cdrcmple
+; WINDOWS-NOT: __aeabi_cdrcmple
+  %retval = alloca i32, align 4
+  %A = alloca double, align 8
+  %B = alloca double, align 8
+  store double 1.000000e+00, double* %A, align 8
+  store double 1.000000e+01, double* %B, align 8
+  %0 = load double, double* %A, align 8
+  %1 = load double, double* %B, align 8
+  %cmp = fcmp oge double %0, %1
+  br i1 %cmp, label %if.then, label %if.end
+
+if.then:
+  store i32 0, i32* %retval, align 4
+  br label %return
+
+if.end:
+  store i32 1, i32* %retval, align 4
+  br label %return
+
+return:
+  %2 = load i32, i32* %retval, align 4
+  ret i32 %2
+}