[PATCH] D38253: [ARM] Restore the right frame pointer register in Int_eh_sjlj_longjmp

Wed Sep 1 00:24:11 PDT 2021

mstorsjo added a reviewer: efriedma.
mstorsjo added a subscriber: xtkoba.
mstorsjo added a comment.
Herald added a project: LLVM.

In D38253#884501 <https://reviews.llvm.org/D38253#884501>, @MatzeB wrote:

> In D38253#883234 <https://reviews.llvm.org/D38253#883234>, @mstorsjo wrote:
>
>> FWIW, this was more or less approved by @t.p.northover on IRC:
>>
>> "The second one is ugly, but that describes SjLj as a whole so I doubt we'd be making the world a worse place." and "There was actually some talk about standardising FP a while ago. Doesn't seem to have gone anywhere yet though."
>
> This is NOT how patch aproval works in llvm!! Please revert!

Really sorry about this - I entirely missed this comment back then... :-( At this point, though, reverting is a bit late/harsh...

> Just glancing at this I can see this changes the asmprinter output but not the modelling of the instruction which now changes r11 in some cases but does not model that fact with machine operands.

A suggested follow-up patch in D109041 <https://reviews.llvm.org/D109041> adds r11 for `tInt_eh_sjlj_longjmp` in ARMInstrThumb.td - does that fix this particular concern (if we'd do the same for `Int_eh_sjlj_longjmp` in ARMInstrInfo.td too)?

================
Comment at: llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp:1207

+  const MachineFunction &MF = *MI->getParent()->getParent();
+  const ARMSubtarget &STI = MF.getSubtarget<ARMSubtarget>();
----------------
MatzeB wrote:
> There’s no need to put those statements on the hot path of this function.
Good point, I'll send a separate patch to fix that, if they aren't used elsewhere now.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D38253/new/

https://reviews.llvm.org/D38253