[PATCH] D14986: Fix Thumb1 epilogue generation

[A. Skrobov via llvm-commits]

tyomitch added inline comments.

================
Comment at: lib/Target/ARM/Thumb1FrameLowering.cpp:413
@@ -412,3 +414,1 @@
-      IsV4PopReturn = true;
-  return IsV4PopReturn && STI.hasV4TOps() && !STI.hasV5TOps();
 }
----------------
jroelofs wrote:
> 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).
If so, it seems it didn't have any regression tests -- so I'm not sure if my patch may lead to generation of the `pop pc` on v4T, or if it may not.

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

================
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));
 
----------------
jroelofs wrote:
> Would this be better written as:
> 
>     Pop = AddDefaultPred(BuildMI(MBB, MBBI, dl, TII.get(ARM::tPOP)))
>               .addReg(PopReg, RegState::Define);
> 
No, because Pop may correspond either to the MachineInstrBuilder from line 519, or to the pre-existing tPOP from line 515.

================
Comment at: test/CodeGen/Thumb/thumb-shrink-wrapping.ll:43
@@ -43,1 +42,3 @@
+; ENABLE-NEXT: pop {r7, r1}
+; ENABLE-NEXT: mov lr, r1
 ;
----------------
jroelofs wrote:
> 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.
> Is the next instruction a bx lr?
Yes it is; but it's also accessible via a by-pass.

Perhaps it would be beneficial to transform a
    pop {r7, r1}
    mov lr, r1
  label:
    bx lr
into
    pop {r7, pc}
  label:
    bx lr
--but that requires a more sophisticated analysis, reaching outside the BB with the epilogue, which emitPopSpecialFixUp() doesn't (and didn't) attempt to do.


http://reviews.llvm.org/D14986