[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:
> 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.
`test/CodeGen/ARM/thumb1_return_sequence.ll` should have been testing it...

Looking at the `svn blame`, I think r242714 broke it. I've replied on that commit's email.

================
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));
 
----------------
tyomitch wrote:
> 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.
Oh, I see.

================
Comment at: test/CodeGen/Thumb/thumb-shrink-wrapping.ll:43
@@ -43,1 +42,3 @@
+; ENABLE-NEXT: pop {r7, r1}
+; ENABLE-NEXT: mov lr, r1
 ;
----------------
tyomitch wrote:
> 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.
Would you mind leaving a FIXME in `emitPopSpecialFixUp` explaining that we have to emit it that way for v4t, but for v5t+ it could be optimized to the shorter sequence?


http://reviews.llvm.org/D14986