In rL351421#306956, @RKSimon wrote:

@ssijaric This is breaking on EXPENSIVE_CHECKS builds - are you looking at this or should I revert?

http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/15202/
http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/15202/steps/test-check-all/logs/stdio

In rL351404#306794, @ssijaric wrote:

In rL351404#306734, @RKSimon wrote:

@ssijaric This is failing on EXPENSIVE_CHECKS buildbots, can you take a look or should I revert?

http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/15196/steps/test-check-all/logs/stdio

Thanks for letting me know! I am looking it at it now.

In rL351404#306734, @RKSimon wrote:

@ssijaric This is failing on EXPENSIVE_CHECKS buildbots, can you take a look or should I revert?

http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/15196/steps/test-check-all/logs/stdio

In D56813#1360762, @dmajor wrote:

I've confirmed that I can build Firefox successfully with this patch on top of latest trunk. Thank you @ssijaric!

Address Eli's comments.

Committed under https://reviews.llvm.org/rL351404. Forgot to add a Different Revision line.

Address formatting comments.

Clarify the comment. By looking at cl.exe generated code, it looks like only the frame pointer is passed in x1. The base pointer, if needed, is used directly (e.g. x19). I will verify with a more complicated test case.

Cleanup per Eli's comments. Checking for the personality function is probably not necessary. I could add an assert instead of the check?

In D51524#1286331, @efriedma wrote:

Oh, I guess there is one special case for the prologue I didn't mention: varargs functions. I think the patch handles that correctly already, but I'll add a test.

For both dynamic allocas and stack realignment, the initial implementation of unwinding missed a significant rule: "r29 must point to the bottom of stack". That's probably straightforward to resolve, but I don't really want to do it in this patch (and I'm not sure what the practical effect is). Beyond that, there's probably more handling necessary to correctly handle a dynamic alloca inside a funclet (since we need to reset the frame pointer, but we still need access to the old frame pointer). And I don't really want to block this patch on solving that; it's obscure enough that it won't affect most code.

I have a couple small fixes for stack realignment, a fix for a MachineVerifier failure, and a testcase for realignment; I'll upload a new version.

Fix the check for IsFunclet in emitEpilogue.

LGTM

Remove an unused variable. Fix a nasty bug that Eli just pointed out, where I didn't insert a PrologEnd in funclets in this patch. Previously, I relied on an RAII object to this for me when we would go out of scope. I removed that logic, but forgot to insert a prolog end marker for the funclet case.

Address comments: spelling mistakes, better comments, a better test case.

Address Eli's feedback.

Add test cases. Use getSEHReg to retrieve SEH register numbers. Clean up the logic in a few places.

Remove spaces in wineh2.mit test case. Fix the extended code words logic that Eli pointed out. Add a test case to make sure that shrink wrapping doesn't kick in.

In D50166#1277766, @rnk wrote:

In D50166#1276921, @ssijaric wrote:

So, I will skip a test case for shrink wrapping. Will add a few more tests to cover remaining the unwind codes.

Can you copy a test where shrink wrapping would fire, and validate that it does not for aarch64-windows-gnu? Without the test, someone could remove the logic you found and no tests would fail. We have a test like this for x86_64.

Add test cases for the remaining SEH opcodes. Disable post RA scheduling for the test cases.

In D50166#1276881, @ssijaric wrote:

In D50166#1274871, @rnk wrote:

In D50166#1273097, @efriedma wrote:

Missing test coverage for SEH_SaveFPLR_X, SEH_SaveReg, SEH_SaveFReg, SEH_SetFP, SEH_AddFP, SEH_Nop, and the alloc_s variant of SEH_StackAlloc. (I'd like to see at least one use of each opcode to show the encoding is correct/consistent with llvm-readobj.)

I'd also like to see a test for shrink-wrapping. You may have to disable it if you cannot yet describe such prologues it with SEH_ opcodes.

Hi Reid,

I think shrink wrapping is disabled by default for AArch64. I have a small test case that gets shrink-wrapped, but only if I enable it manually using -enable-shrink-wrap. Do we need a test case if it's disabled by default?

In D50166#1274871, @rnk wrote:

In D50166#1273097, @efriedma wrote:

Missing test coverage for SEH_SaveFPLR_X, SEH_SaveReg, SEH_SaveFReg, SEH_SetFP, SEH_AddFP, SEH_Nop, and the alloc_s variant of SEH_StackAlloc. (I'd like to see at least one use of each opcode to show the encoding is correct/consistent with llvm-readobj.)

I'd also like to see a test for shrink-wrapping. You may have to disable it if you cannot yet describe such prologues it with SEH_ opcodes.

We need to have a test case that checks for access to local variables from the finally and except funclets. The parent's frame pointer is the second argument to both the filter functions and termination handlers, fro what I see. We need to make sure that locals variables in funclets are being accessed off x1, and not the stack pointer.

Fix the test case, wineh5.mir, to conform to what cl.exe produces.

Re-uploaed with full context and remove an unused declaration of getLabelPlusOne.

Add test cases. These depend on D53264 and D51201.

Fix the printing of save_freg and save_freg_x that Eli just pointed out.

Address Eli's latest comments.

Fix a few operator precedence bugs uncovered while writing test cases for the MCLayer patch (D50166).

Also fix a few unwind code hex dumps. Wasn't printing all the bytes.

Add hasWinCFI to MIRPrinter as well, now that HasWinCFI is a plain bool.

Better error handling. Fix alloc_m decoding.

Address Eli's latest feedback, and an add an additional test case.

Fix bugs and add two assembly test cases constructed by hand to test the error handling.

Addressed Eli's comments.

Thanks for reviewing, Reid. This update uses llvm_unreachable instead of asserts, and moves createAArch64TargetObjectStreamer to AArch64TargetStreamer.h/.cpp. To do this, I also moved AArch64TargetELFStreamer to the llvm namespace.

Addressed Francis' review feedback, and ran clang-format through some parts of the changes in AArch64FrameLowering.cpp. Changed the title to better reflect the intent of the patch.

Thanks for reviewing, Francis. I'll upload an updated patch.

A few formatting fixes.

Addressed the comments and reduced the patch to handle only the frame lowering portion. Changes for funclets and EH tables will go in separate patches.

I also moved the AArch64 specific changes to the Windows exception handling tables to this patch, as they are needed to get the test case to pass (they are required in order to populate .xdata correctly). The test case also has a dependence on https://reviews.llvm.org/D51201, which adds HasWinCFI to the MIRParser.

Add a test case and fix a couple of encodings.

Move SEH opcode handling to AArch64 target streamers.

Hi Reid,

Remove a couple of unused functions from the initial patch. Fix formatting.

Use errs instead of assert MCAsmPrinter for unwind opcodes that are not yet supported. Address Eli's non-determinism comment. Use range-based for loops. Still need to add a test case.

Last change before committing to address Reid's point of using CCK_MicrosoftWin64 for Windows on both X86_64 and ARM64.

Updated to address Martin's observation. Verified that small structs with default copy constructors and non-trivial destructors are passed in registers on ARM64 Windows.

Yes, it looks like the comment for the thumb case is applicable here as well. VC++ on ARM64 passes small structs with a destructor and a default copy constructor in a register, while clang passes them on the stack. I will probably address this in a separate patch.

LGTM

Abandoning, as this will be fixed in D49644.