[PATCH] D34475: [AArch64] Add support for __builtin_ms_va_list on aarch64

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: include/clang/Basic/BuiltinsAArch64.def:65
+// Win64-compatible va_list functions
+BUILTIN(__builtin_ms_va_start, "vc*&.", "nt")
+BUILTIN(__builtin_ms_va_end, "vc*&", "n")
----------------
I strongly suspect that Microsoft will never adopt a varargs calling convention that uses a complex, non-`char*` va_list.

I'm starting to think we should move this to the generic builtin list and make it available everywhere. The semantics are that you can only use __builtin_ms_va_start in ms_abi functions. It always produces a `char*`-style va_list.



================
Comment at: include/clang/Basic/Specifiers.h:239
     CC_X86Pascal,   // __attribute__((pascal))
     CC_X86_64Win64, // __attribute__((ms_abi))
     CC_X86_64SysV,  // __attribute__((sysv_abi))
----------------
I think we might prefer to make this non-x86_64 specific. I suspect that this pattern will arise again on ARM32 if anyone goes back there in seriousness. We'll probably want sysv_abi as well as ms_abi, and all the logic should be independent of the ISA: ms_abi is a no-op when the target OS is already Windows, and sysv_abi is a no-op when the target OS isn't Windows.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:5276
                                                const CallExpr *E) {
+  if (BuiltinID == AArch64::BI__builtin_ms_va_start ||
+      BuiltinID == AArch64::BI__builtin_ms_va_end)
----------------
If we made __builtin_ms_va_start generic, that would also eliminate this duplicate code.


================
Comment at: lib/CodeGen/CGCall.cpp:223-224
   if (D->hasAttr<MSABIAttr>())
-    return IsWindows ? CC_C : CC_X86_64Win64;
+    return IsWindows ? CC_C : Arch == llvm::Triple::aarch64 ? CC_AArch64Win64
+                                                            : CC_X86_64Win64;
 
----------------
Unifying the ms_abi CCs would remove the need for this.


================
Comment at: lib/Sema/SemaChecking.cpp:3625
 /// target and calling convention.
 static bool checkVAStartABI(Sema &S, unsigned BuiltinID, Expr *Fn) {
   const llvm::Triple &TT = S.Context.getTargetInfo().getTriple();
----------------
Oh dear, how can we keep this simple. I think unifying the CC's would improve things.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:4283
+
+    CC = Context.getTargetInfo().getTriple().isOSWindows()
+             ? CC_C
----------------
Ditto


https://reviews.llvm.org/D34475