Thanks, LGTM.

Probably D18619 doesn't handle vcall thunk as below correctly when stack bump is not combined (in convertCalleeSaveRestoreToSPPrePostIncDec). The produced code has the last second add instruction removed which leaves unbalanced stack.

LGTM.

Thanks for catching this. Could the #pragma push and #undef be moved to the line right before CV_REGISTER(ARM64_FPSR, 220). An inline commend will also be great.

The fix in LLVM was merged as https://reviews.llvm.org/rL364115.

Thanks Reid and Eli for the catch. Added -fast-isel and -global-isel to test configurations, and also added the implementation for both fast isel and global isel.

In D63617#1552615, @rnk wrote:

Even if BRK #0xF000 is a Windows convention, it's still possible for ISel to select different instructions for different OSs, and I'd prefer to implement it that way.

In D63617#1552561, @rnk wrote:

I think it would be preferable to make llvm.debugtrap emit brk #0xF000 on aarch64-windows-*, so other frontends (Rust etc) get the right behavior by default. Right now, AArch64 doesn't do anything special for DEBUGTRAP, so we get the default behavior of llvm.trap.

Is this BRK #0xF000 convention universal to all OSs, or is it a specific convention for Windows debuggers?

In D62608#1526620, @mgorny wrote:

Could we, please, revert this until the fix for LLDB is ready and approved?

The fix for LLDB was merged yesterday.

In D62772#1526824, @compnerd wrote:

This should get the build working again, so lets get this fixed, we can improve it later

Change to switch/case based on comment.

In D62772#1526145, @mgorny wrote:

Are you sure this triple → CPUType mapping belongs in each consumer? Maybe it'd be better to have something inside LLVM, so that we wouldn't have to keep this up-to-date in all the places. Maybe getRegisterNames() overload that takes a triple?

Update variable naming to be consistent with current file.

In D62608#1526080, @mgorny wrote:

This API change broke builds of LLDB:
http://green.lab.llvm.org/green/job/lldb-cmake/27531/
http://lab.llvm.org:8011/builders/netbsd-amd64/builds/20296

Remove unnecessary const and clean up.

It makes sense to fix the dumper for test infrastructure. This update fixes register names in the dumper. I also changed the wildcard for register name to ARM64_SP in the new test, because register name is shown correct by the dumper based on CPUType in the PDB.

In D62608#1522078, @rnk wrote:

In D62608#1521945, @efriedma wrote:

The code looks fine now. I'm okay with putting off fixes for dumping code that incorrectly assumes x86.

The dumping tools are the foundation of our testing strategy, so I think we should take the time to fix them early on. It should be a matter of changing getRegisterNames to accept a CPU type, switch, and return the appropriate (x86 or arm) enum table, maybe just returning x86 by default to preserve existing behavior. Both symbol dumpers already store the compilation CPU type from the S_COMPILE3 record, so this shouldn't be too complicated.

Did a little cleanup as per Eli's comment (removed attributes and llvm.ident. Tried some others but got llc failure, so I'd like to be conservation on this. Also the test follows register-variables.ll in the same folder which keeps all the metadata.

Added CV_REGISTERS_{ARCH} macro for each architecture, and fixed typo.

Added test cases and also merged this alignment adjustment to getMinGlobalAlign in MicrosoftARM64TargetInfo.

In D60349#1479604, @efriedma wrote:

Return info for HFA and HVA is updated

That's helpful, but not really sufficient; according to the AAPCS rules, both "Pod" and "NotPod" from my testcase are HFAs.

In D60349#1477770, @TomTan wrote:

In D60349#1477183, @efriedma wrote:

For NotPod, it is aggregate which is specific in the document

Yes, it's an aggregate which is returned in registers... but it's returned in integer registers, unlike Pod which is returned in floating-point registers.

struct Pod is HFA (Homogenous Floating-Point Aggregates) which is returned in floating-point register, struct NotPod is not HFA so still returned in integer registers. The ARM64 ABI document will be updated to reflect this, thanks.

In D60349#1477183, @efriedma wrote:

For NotPod, it is aggregate which is specific in the document

Yes, it's an aggregate which is returned in registers... but it's returned in integer registers, unlike Pod which is returned in floating-point registers.

In D60349#1476371, @efriedma wrote:

It looks like there's some missing documentation in the ARM64 ABI document involving homogeneous aggregates... in particular, it looks like non-POD types are never homogeneous, or something along those lines. I guess we can address that in a followup, though.

@TomTan could you look into updating the ARM64 ABI documentation?

Testcase:

struct Pod {
  double b[2];
};
struct NotAggregate {
  NotAggregate();
  double b[2];
};
struct NotPod {
  NotAggregate x;
};
Pod copy(Pod *x) { return *x; }  // ldp d0,d1,[x0]
NotAggregate copy(NotAggregate *x) { return *x; } // stp x8,x9,[x0]
NotPod copy(NotPod *x) { return *x; } // ldp x0,x1,[x8]

In D57915#1393510, @thakis wrote:

In D57915#1389722, @TomTan wrote:

In D57915#1389560, @lebedev.ri wrote:

In D57915#1389549, @TomTan wrote:

Added the tests back. Clang IR should not lower these to bswap calls because they are global library functions. It might be slower to make the call to library function than bswap, but this is the same for other architectures supported by Windows. And just redefine global library function triggers link error in some scenarios.

I think there is some deeper reasoning is being omitted here.
Why does the fact that those are "global library functions" bans clang from lowering them into IR?
(and thus, "prevents" LLVM form doing optimizations)

The current issue could be caused by the implementation of comdat selection in LLD as below which provides more strict conflict check than MSVC link does.

lld isn't supposed to be more strict than link.exe. lld used to not implement the comdat selection field until recently, so lld got more strict – but it should've gotten only as strict as link.exe, not moreso. Do you have an example where lld is more strict than link.exe?

In D57915#1389788, @efriedma wrote:

I did some quick testing with MSVC; apparently it inlines the implementations of these functions when optimizations are on. We definitely want to support inlining these. Since these are commonly used in performance-sensitive code, I'd prefer to implement the required changes to CodeGenFunction::EmitMSVCBuiltinExpr now, rather than chase after weird performance regressions in the future.

---

I'm not sure how you could end up with a "duplicate symbols" error from the current implementation, though; these functions are marked "static", so they shouldn't conflict with the real _byteswap_* functions.

In D57915#1389750, @lebedev.ri wrote:

In D57915#1389722, @TomTan wrote:

In D57915#1389560, @lebedev.ri wrote:

In D57915#1389549, @TomTan wrote:

Added the tests back. Clang IR should not lower these to bswap calls because they are global library functions. It might be slower to make the call to library function than bswap, but this is the same for other architectures supported by Windows. And just redefine global library function triggers link error in some scenarios.

I think there is some deeper reasoning is being omitted here.
Why does the fact that those are "global library functions" bans clang from lowering them into IR?
(and thus, "prevents" LLVM form doing optimizations)

The current issue could be caused by the implementation of comdat selection in LLD as below which provides more strict conflict check than MSVC link does.

These _bytewap_* in intrin.h were not intended as optimization, instead, it is expected to be consistent with declarations in stdlib.h. For optimization, it makes sense to enable it for all architectures supported by Windows, and make sure it works with LLD.

https://github.com/llvm-mirror/lld/blob/b6584c3fab115aa46dad27681af7eb3d4e5d2b35/COFF/InputFiles.cpp#L494

These functions are also listed in https://github.com/llvm-mirror/clang/blob/c18e7e9007970a3105617f03bc9d1de89fa1a3e1/include/clang/Basic/Builtins.def#L773-L775
Are you sure they shouldn't be simply dropped from lib/Headers/intrin.h?

I notice they were just added in D56685, but that review has no commit message, so the problem it addressed is not documented..
So as-is this all looks a bit like hand-waving to me.

In D57915#1389560, @lebedev.ri wrote:

In D57915#1389549, @TomTan wrote:

Added the tests back. Clang IR should not lower these to bswap calls because they are global library functions. It might be slower to make the call to library function than bswap, but this is the same for other architectures supported by Windows. And just redefine global library function triggers link error in some scenarios.

I think there is some deeper reasoning is being omitted here.
Why does the fact that those are "global library functions" bans clang from lowering them into IR?
(and thus, "prevents" LLVM form doing optimizations)

Added the tests back. Clang IR should not lower these to bswap calls because they are global library functions. It might be slower to make the call to library function than bswap, but this is the same for other architectures supported by Windows. And just redefine global library function triggers link error in some scenarios.

Thanks for finding out and fixing this. Seems there is also issue in expanding _WriteStatusReg in CodeGenFunction::EmitAArch64BuiltinExpr. The last argument for _WriteStatusReg is zero extended to __in64, which is not expected (see below link).

In D57631#1381806, @efriedma wrote:

LGTM

In D57183#1378121, @mgrang wrote:

With this patch, most of the SEH tests in https://github.com/Microsoft/windows_seh_tests/blob/master/src/xcpt4/xcpt4u.c pass.

Added COFF target check.

Thanks for the review, Martin. Could you please help commit this? I don't have commit access.

Added CHECK-NEXT on the jump table label based on Martin's feedback.

Thanks Peter and Martin. Yes, IMAGE_REL_ARM64_REL32 can do the relocation work here. I saw Martin added this relocation type to both LLVM and LLD. My change in AArch64AsmPrinter::EmitJumpTableInfo could be reverted when IMAGE_REL_ARM64_REL32 is attached to the jump table entries, but the relocation model is PIC for Windows ARM64.

In D56813#1360814, @ssijaric wrote:

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!

Hi David,

Can you please try with the latest version of the patch? I am going to commit it shortly.

Thanks.

We need full definition for __nop in intrin.h.

Hi Zachary, does this line "#include "Windows/WindowsSupport.h" " need to be changed to "#include "WindowsSupport.h" " after it is moved from Threading.cpp to Threading.inc, because Threading.inc which includes WindowsSupport.h is under Windows folder, so no need to have it in the include path. This current include path could trigger warning "#include resolved using non-portable Microsoft search rules as: ..."

In D53540#1326343, @mgrang wrote:

@dmajor My updated change fixes the crash in your testcase.

Now, I am trying to build the SEH tests released by MS: https://github.com/Microsoft/windows_seh_tests. I started with XCPT4 tests. test5 is currently failing which I am triaging now.