[PATCH] D77016: Change AArch64 Windows EH UnwindHelp object to be a fixed object

[Daniel Frampton via Phabricator via llvm-commits]

danielframpton marked 6 inline comments as done.
danielframpton added a comment.

Added responses to the review and pointed out places where there are important codegen differences.



================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:219
 
+/// Returns the size of fixed object (to determine the prologue size).
+static unsigned getFixedObjectSize(const MachineFunction &MF,
----------------
efriedma wrote:
> Maybe elaborate a bit; this is specifically the first space allocated in the function, next to sp on entry.
Is this good?


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1011
       Subtarget.isCallingConvWin64(MF.getFunction().getCallingConv());
   // Var args are accounted for in the containing function, so don't
   // include them for funclets.
----------------
efriedma wrote:
> Maybe move the "Var args are accounted for in the containing function" comment into getFixedObjectSize?
Done.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:2683
+  for (int I = MFI.getObjectIndexBegin(); I < 0; ++I)
+    MinFixedObjOffset = std::min(MinFixedObjOffset, MFI.getObjectOffset(I));
+
----------------
efriedma wrote:
> This loop is strange. There's space explicitly allocated for the object in getFixedObjectSize, so you should be able to directly compute its offset.
This loop exists in the corresponding X86 code, but the use of FixedObject there is different.

I have changed to use the calculation from above instead.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:2690
+  // Ensure alignment.
+  UnwindHelpOffset = -alignTo(-UnwindHelpOffset, 16);
+
----------------
efriedma wrote:
> Why does UnwindHelp need to be 16-byte aligned?
The unwind help object itself only needs 8 byte alignment, but the code was expecting the FixedObject area to be 16 byte aligned.


================
Comment at: llvm/test/CodeGen/AArch64/seh-finally.ll:94
 ; CHECK: mov     x0, #-2
-; CHECK: stur    x0, [x19, #16]
-; CHECK: .set .Lstack_realign$frame_escape_0, 32
-; CHECK: ldr     w0, [x19, #32]
+; CHECK: stur    x0, [x29, #32]
+; CHECK: .set .Lstack_realign$frame_escape_0, 0
----------------
This is the key difference in codegen (using fp to store the -2).


================
Comment at: llvm/test/CodeGen/AArch64/seh-finally.ll:213
 ; CHECK: mov     x1, #-2
-; CHECK: stur    x1, [x19]
+; CHECK: stur    x1, [x29, #32]
 ; CHECK: .set .Lvla_and_realign$frame_escape_0, 32
----------------
This is the key difference in codegen (using fp to store the -2).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77016