[PATCH] D125648: [ARM SEH 6] [ARM] Add SEH opcodes in frame lowering

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMFrameLowering.cpp:315
+  case ARM::t2MOVi16: { // mov(w) r4, #xx
+    bool Wide = MBBI->getOperand(1).getImm() >= 256;
+    MIB = BuildMI(MF, DL, TII.get(ARM::SEH_Nop)).addImm(Wide).setMIFlags(Flags);
----------------
efriedma wrote:
> Looking at this again, this is actually sort of scary.  In particular, this is dependent on looking into the future: trying to predict what Thumb2SizeReduction will do with a given instruction. Which is at best fragile, at worst broken if Thumb2SizeReduction doesn't run, or decides to do something different.
> 
> I guess you can sort of predict what will happen for t2MOVi16 and t2LDMIA_RET/t2LDMIA_UPD/t2STMDB_UPD.  But it's less clear in other cases; we currently don't optimize t2SUBspImm, but we could.  Or for TCRETURNdi, we don't actually decide the size until we hit the assembler.
> 
> I'm thinking we might want to disable Thumb2SizeReduction on instructions with SEH opcodes.  (Or equivalently, on FrameSetup instructions if SEH unwind is enabled.)
Thanks - this was indeed one of my fears initially. In practice, these guesses for what it will end up like have worked for all the code I've tested this on so far. But it's indeed brittle.

Skipping Thumb2SizeReduction for FrameSetup/FrameDestroy when SEH unwind is enabled seems to work fine though, so that alleviates most of the issue. (As a future TODO, one could maybe consider rewriting the MI to a narrow form already at this point, for the few opcodes where it matters?)

For TCRETURNdi, I also feared that it would be an issue, but it hasn't cropped up. (Or maybe the nondeterminate length of the instruction makes it unable to calculate the length of the epilogue at that point? And thus just skips the check...)

But it seems like the pseudo expansion of TCRETURNdi already has got such a case; MachO also requires strictly Thumb2 wide branches for tail calls, so we can opt in to that logic for SEH too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125648