[PATCH] D14986: Fix Thumb1 epilogue generation

[Jonathan Roelofs via llvm-commits]

jroelofs added inline comments.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:413
@@ -412,3 +414,1 @@
-      IsV4PopReturn = true;
-  return IsV4PopReturn && STI.hasV4TOps() && !STI.hasV5TOps();
 }
----------------
tyomitch wrote:
> This must have been a leftover from some ARM lowering code: `hasV4TOps()` and `hasV5TOps()` have no relevance to Thumb1 epilogue generation, but this code used to prevent the "pop special fix-up" when it was necessary.
IIRC, this check was to make sure that we didn't emit a `pop {r[0-9], pc}` on v4t, and expect it to change the thumb bit accordingly (because it can't).

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:517
@@ +516,3 @@
+    assert(Pop->getOpcode() == ARM::tPOP);
+    Pop->RemoveOperand(Pop->findRegisterDefOperandIdx(ARM::LR));
+  } else
----------------
Should assert that findRegisterDefOperandIdx didn't return `-1`.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:520
@@ -514,1 +519,3 @@
+    Pop = AddDefaultPred(BuildMI(MBB, MBBI, dl, TII.get(ARM::tPOP)));
+  Pop->addOperand(MachineOperand::CreateReg(PopReg, true));
 
----------------
Would this be better written as:

    Pop = AddDefaultPred(BuildMI(MBB, MBBI, dl, TII.get(ARM::tPOP)))
              .addReg(PopReg, RegState::Define);


================
Comment at: test/CodeGen/Thumb/thumb-shrink-wrapping.ll:43
@@ -43,1 +42,3 @@
+; ENABLE-NEXT: pop {r7, r1}
+; ENABLE-NEXT: mov lr, r1
 ;
----------------
Is the next instruction a `bx lr`?


If so, it'd be better to emit:

    blx r1

on v5t+, and:

    mov lr, r1
    bx lr

on v4t.


http://reviews.llvm.org/D14986