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

[Saleem Abdulrasool]

Id say merge all the test cases into a single file (thumb1-tail-call.ll ?).


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:1677
@@ +1676,3 @@
+  bool IsR3UsedForArgumentPassing = false;
+  if (RegsToPass.size() >= 4) {
+    IsR3UsedForArgumentPassing = true;
----------------
Unnecessary braces

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:1683
@@ +1682,3 @@
+
+  if (isTailCall && IsR3UsedForArgumentPassing && Subtarget->isThumb1Only() )
+    ForceCallAddrToRegR12 = true;
----------------
Unnecessary whitespace before the last ).  Why not collapse this to:

    bool ForceCallAddrToIP = isTailCall && (RegsToPass.size() >= 4 && Subtarget->isThumb1Only());

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:1694
@@ -1681,3 +1693,3 @@
 
-  if (EnableARMLongCalls) {
+  if (EnableARMLongCalls || (isTailCall && Subtarget->isThumb1Only() )) {
     assert((Subtarget->isTargetWindows() ||
----------------
Unnecessary whitespace in condition.

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:1805
@@ +1804,3 @@
+                             Callee,Chain.getValue(1));
+    Callee = DAG.getRegister (ARM::R12,getPointerTy());
+  }
----------------
Unnecessary whitespace between getRegister and (.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:335
@@ +334,3 @@
+  if (MBBI->getOpcode() == ARM::TCRETURNri)
+	  IsTailCallReturn = true;
+
----------------
Why not collapse this to:

    bool IsTailCallReturn = MBBI->getOpcode() == ARM::TCRETURNri;

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:361
@@ -356,1 +360,3 @@
+    // Unwind MBBI to point to first LDR / VLDRD. Not for tail call returns!
+    if ((MBBI != MBB.begin()) && (!IsTailCallReturn)) {
       do
----------------
I don't think that the parenthesis are needed and they detract from readability.  I would swap the order since the negation check would short-circuit.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:403
@@ -394,1 +402,3 @@
+      }
+        else if (!tryFoldSPUpdateIntoPushPop(STI, MF, MBBI, NumBytes))
         emitSPUpdate(MBB, MBBI, TII, dl, *RegInfo, NumBytes);
----------------
This should be on the same line as the brace.  Can we perhaps fold the knowledge that the fold should be avoided in tail calls into tryFoldSPUpdateIntoPushPop?  At the very least, I think the condition should be:

    else if (IsTailCallReturn || !tryFoldSPUpdateIntoPushPop(...))

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:413
@@ +412,3 @@
+  for (const CalleeSavedInfo &CSI : MFI->getCalleeSavedInfo()) {
+    if (CSI.getReg() == ARM::R4)
+      IsR4InCSI = true;
----------------
Use a switch rather than the if cases?

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:428
@@ +427,3 @@
+    // this is *not* done in restoreCalleeSavedRegisters.
+    const std::vector<CalleeSavedInfo> &CSI(MFI->getCalleeSavedInfo());
+
----------------
Any reason to not use '='?  That is the general style in LLVM.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:441
@@ +440,3 @@
+      MachineOperand &Operand = MBBI->getOperand(i);
+      if (Operand.isReg()) {
+        if (Operand.getReg() == ARM::R3)
----------------
Unnecessary braces.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:447
@@ +446,3 @@
+
+    if (IsLRInCSI && ! IsR3AvailableAsSpill) {
+      // We need to restore LR before pop
----------------
Unnecessary space after the !.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:451
@@ +450,3 @@
+      int StackSlotForSavedLR = CSI.size() - 1;
+      assert (StackSlotForSavedLR >= 0 && "Wrong Stack slot for LR.");
+
----------------
Unnecessary space after assert.  I would also drop the >= 0.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:466
@@ +465,3 @@
+        AddDefaultPred(BuildMI(MBB, MBBI, dl, TII.get(ARM::tPUSH)))
+            .addReg(ARM::R4,RegState::Kill);
+
----------------
Space after the comma.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:468
@@ +467,3 @@
+
+        StackSlotForSavedLR ++;
+      }
----------------
Unnecessary space before the ++.  Actually, switch to the prefixed form as that is preferred in LLVM code style.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:511
@@ +510,3 @@
+    if (IsLRInCSI) {
+      const Thumb1RegisterInfo *RegInfo =
+          static_cast<const Thumb1RegisterInfo *>
----------------
Perhaps const auto *RI would help reduce the wrapping?

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:527
@@ +526,3 @@
+
+        if (ArgRegsSaveSize) {
+          emitSPUpdate(MBB, MBBI, TII, dl, *RegInfo, ArgRegsSaveSize);
----------------
Unnecessary braces

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:539
@@ +538,3 @@
+
+    assert (MBBI->getOpcode() == ARM::TCRETURNri);
+    DebugLoc dl = MBBI->getDebugLoc();
----------------
unnecessary space after assert.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:542
@@ +541,3 @@
+
+    BuildMI(MBB, MBBI, dl,
+            TII.get(ARM::tTAILJMPr))
----------------
The next line should fit on this line.  Why the unnecessary dl variable declaration above?  Its a single use, why not inline it here.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:656
@@ -503,1 +655,3 @@
                             const TargetRegisterInfo *TRI) const {
+
+  MachineBasicBlock::iterator MBBI = MBB.getLastNonDebugInstr();
----------------
Unnecessary empty line.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:660
@@ +659,3 @@
+  if(MBBI->getOpcode() == ARM::TCRETURNri)
+    return true; // Handle pop generation in emitEpliogue
+
----------------
Seems like this could be written as:

    if (MBB.getLastNonDebugInstr()->getOpcode() == ARM::TCRETURNri)
      return true;

================
Comment at: lib/Target/ARM/Thumb1RegisterInfo.cpp:425
@@ +424,3 @@
+  // Both are call clobbered register that otherwise won't be used in
+  // Thumb1 mode.
+
----------------
The text feels weird.  Reflow the text?

================
Comment at: lib/Target/ARM/Thumb1RegisterInfo.cpp:431
@@ +430,3 @@
+  bool IsLRInCSI = false;
+  for (const CalleeSavedInfo &CSI : MFI->getCalleeSavedInfo()) {
+    if (CSI.getReg() == ARM::LR)
----------------
Unnecessary braces.

================
Comment at: lib/Target/ARM/Thumb1RegisterInfo.cpp:436
@@ +435,3 @@
+
+  unsigned ScavengeReg = ARM::R12;
+  if (IsLRInCSI && MBB.isLiveIn(ScavengeReg) && !MFI->isReturnAddressTaken())
----------------
Is ScratchReg a better name perhaps?

http://reviews.llvm.org/D7005

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