[PATCH] Add tail call optimization for thumb1-only targets

[Jonathan Roelofs]

I think you should also add tests for tail-called functions that return things other than void (especially things wider than one register in size).

One more question: do these changes still work on armv4t when an arm function tail calls a thumb function (and vice versa)?


REPOSITORY
  rL LLVM

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:1682
@@ +1681,3 @@
+  bool IsCallAddressMoveToRegisterRequired = false;
+  bool CallAdressShallBeForcedToHardRegR12 = false;
+
----------------
as mentioned in the other thread, these variable names feel quite long.

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:1688
@@ +1687,3 @@
+
+	  if (isTailCall
+	      && IsR3UsedForArgumentPassing
----------------
I think the usual "llvm style" here is to put the &&'s at the end of the line, rather than at the beginning, and only break the line where it would go over 80 cols.

Might be worthwhile to clang-format the patch.

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:1720
@@ -1697,2 +1719,3 @@
       SDValue CPAddr = DAG.getTargetConstantPool(CPV, getPointerTy(), 4);
+
       CPAddr = DAG.getNode(ARMISD::Wrapper, dl, MVT::i32, CPAddr);
----------------
unncessary whitespace

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:1722
@@ -1698,2 +1721,3 @@
       CPAddr = DAG.getNode(ARMISD::Wrapper, dl, MVT::i32, CPAddr);
+
       Callee = DAG.getLoad(getPointerTy(), dl,
----------------
ditto

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:418
@@ -402,1 +417,3 @@
+
+  bool IsV4PopReturn = IsLRInCSI;
   IsV4PopReturn &= STI.hasV4TOps() && !STI.hasV5TOps();
----------------
would be better now to combine this line with the one below it.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:433
@@ +432,3 @@
+    // for tail calls requires R4 as scratch register.
+    bool IsR4ToBeAdditionallyAddedToPopIns = false;
+
----------------
Shorter name suggestion: `MustRestoreR4`.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:445
@@ +444,3 @@
+
+    bool IsLRRestoreAfterPop = IsR3AvailableAsSpill;
+
----------------
This variable is never assigned to again.... Should pick one name for this, and stick with it, delete the other one.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:452
@@ +451,3 @@
+
+      unsigned RegToUseForLRRestore;
+
----------------
A shorter name suggestion: `LRRestoreReg`.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:461
@@ +460,3 @@
+      else {
+        IsR4ToBeAdditionallyAddedToPopIns = true;
+
----------------
Need a `RegToUseForLRRestore = ARM::R4;` here, otherwise it is used uninitialized later.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:482
@@ +481,3 @@
+
+    bool NumRegs = false;
+
----------------
I think a better name for this would be "EmptyPop", with all of it's defs & uses inverted.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:658
@@ +657,3 @@
+  // We will handle callee saving in emitEpilogue and not here.
+  if (IsTailCallReturn)
+    return true;
----------------
Get rid of the variable `IsTailCallReturn` and just check the condition here.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:680
@@ -525,1 +679,3 @@
+      // ARMv4T require BX, see emitEpilogue
+      if ((STI.hasV4TOps() && !STI.hasV5TOps()))
         continue;
----------------
changes here do nothing... would be better to revert them. same with the added newlines.

================
Comment at: test/CodeGen/ARM/twoParametersTailCall_v6m.ll:19
@@ +18,3 @@
+
+attributes #0 = { nounwind "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
----------------
I think all of the attributes and debug information can be dropped. Most tests put the `CHECK:` lines inbetween the `ret` and the '}' at the end of the function.

http://reviews.llvm.org/D7005

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/