[clang] Fix reading long doubles with va_arg on x86_64 mingw

[mstorsjo]

Summary

On x86_64 mingw, long doubles are always passed indirectly as
arguments (see an existing case in WinX86_64ABIInfo::classify); we
need to take this into account when manually reading back arguments
with va_arg too.

[rnk]

clang/lib/CodeGen/TargetInfo.cpp

4361–4362

I wonder if we should make the check more general. There are other cases to consider, like __int128. How does GCC pass other large types through varargs? It looks to me like clang passes those indirectly, so we could go ahead and check the type size directly without these aggregate checks.

Separately, I think X86_64ABIInfo::EmitMSVAArg has a bug, it always passes false for indirect, but it should match this code here exactly. Ideally, they would share an implementation.

[mstorsjo]

clang/lib/CodeGen/TargetInfo.cpp

4361–4362

I'll have a look at what GCC does for __int128 yeah.

Yes, X86_64ABIInfo::EmitMSVAArg is lacking, I've got a separate patch fixing that one up to be posted after this one (but this patch fixes an actual issue I've run into, the other one is more of a correctness thing).

The open question about that one, is what to do about long double. If using __attribute__((ms_abi)) and __builtin_ms_va_list on a non-windows platform, we don't know if they're meaning to interact with the MS or GNU ABI regarding long doubles.

On one hand, one would mostly be using those constructs to reimplement Windows API (i.e. implementing wine or something similar), and in that case, only the MS behaviour is of interest. On the other hand, mapping va_arg(ap, long double) to the GNU case doesn't take anything away for the user either, because if you want a MSVC long double, you can just do va_arg(ap, double). Then finally, I guess there's no guarantee that the platform where doing that even has an exactly matching long double?

[mstorsjo]

clang/lib/CodeGen/TargetInfo.cpp

4361–4362

GCC does the same for __int128 in varargs as it does for long double, so that does indeed simplify the code a fair bit.

And I also realized it's not a problem wrt long double and the __builtin_ms_va_list; if the caller does va_arg(ap, long double), that has to be handled according to whatever the size of long double is on platform, which hopefully would match what GCC uses on windows.

[mstorsjo]

Updated to not require the types to be either isAggregateTypeForABI(Ty) or Ty->isMemberPointerType(), just check the size of the type, added a testcase for __int128 (which I tested against GCC).

[mstorsjo]

Updated the CodeGenCXX/ext-int.cpp testcase.

[rnk]

lgtm

clang/test/CodeGen/mingw-long-double.c

56–58

These tests will stop working after opaque pointers happens, which I hope comes in the next year. If you look for a load of the pointer type, that should be resilient to opaque pointers.

clang/test/CodeGen/win64-i128.c

25

ditto

[mstorsjo]

clang/test/CodeGen/mingw-long-double.c

56–58

Hmm, good point, but the test feels more brittle to me in that form:

// GNU32: load x86_fp80, x86_fp80*
// GNU64: load x86_fp80*, x86_fp80**
// GNU64: load x86_fp80, x86_fp80*
// MSC64: load double, double*

That wouldn't notice if GNU32 also used an indirect pointer for this case (as it would still match the second load - but it would notice if GNU64 stopped using an indirect pointer as the first load wouldn't be matched). I guess it'd be safer if I'd use more named regex patterns in the test to follow the chain from the argp, but then we end up with a pattern covering the bits that change due to opaque pointers too.

[mstorsjo]

clang/test/CodeGen/mingw-long-double.c

56–58

@rnk - WDYT about how to write the test patterns - see above?

[rnk]

clang/test/CodeGen/mingw-long-double.c

56–58

I guess the only two ideas I have are to try out update_cc_test_checks.py or to CHECK-NOT for the indirect load.

[mstorsjo]

clang/test/CodeGen/mingw-long-double.c

56–58

Ok, CHECK-NOT sounds like a sensible compromise in making it more robust.

clang/lib/CodeGen/TargetInfo.cpp

if (IsVectorCall) {
     // second pass.
     for (auto &I : FI.arguments())
       I.info = reclassifyHvaArgForVectorCall(I.type, FreeSSERegs, I.info);
   }
 }
 
 Address WinX86_64ABIInfo::EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
                                     QualType Ty) const {
-
-  bool IsIndirect = false;
-
   // MS x64 ABI requirement: "Any argument that doesn't fit in 8 bytes, or is
   // not 1, 2, 4, or 8 bytes, must be passed by reference."
-  if (isAggregateTypeForABI(Ty) || Ty->isMemberPointerType()) {
-    uint64_t Width = getContext().getTypeSize(Ty);
-    IsIndirect = Width > 64 || !llvm::isPowerOf2_64(Width);
-  }
+  bool IsIndirect = Width > 64 || !llvm::isPowerOf2_64(Width);
 
   return emitVoidPtrVAArg(CGF, VAListAddr, Ty, IsIndirect,
                           CGF.getContext().getTypeInfoInChars(Ty),
                           CharUnits::fromQuantity(8),
                           /*allowHigherAlign*/ false);
 }
 
 static bool PPC_initDwarfEHRegSizeTable(CodeGen::CodeGenFunction &CGF,

clang/test/CodeGen/mingw-long-double.c

 }
 // GNU32: define dso_local void @TestLDC({ x86_fp80, x86_fp80 }* noalias sret({ x86_fp80, x86_fp80 }) align 4 %agg.result, { x86_fp80, x86_fp80 }* byval({ x86_fp80, x86_fp80 }) align 4 %x)
 // GNU64: define dso_local void @TestLDC({ x86_fp80, x86_fp80 }* noalias sret({ x86_fp80, x86_fp80 }) align 16 %agg.result, { x86_fp80, x86_fp80 }* %x)
 // MSC64: define dso_local void @TestLDC({ double, double }* noalias sret({ double, double }) align 8 %agg.result, { double, double }* %x)
 
 // GNU32: declare dso_local void @__mulxc3
 // GNU64: declare dso_local void @__mulxc3
 // MSC64: declare dso_local void @__muldc3
+
+void VarArgLD(int a, ...) {
+  // GNU32-LABEL: define{{.*}} void @VarArgLD
+  // GNU64-LABEL: define{{.*}} void @VarArgLD
+  // MSC64-LABEL: define{{.*}} void @VarArgLD
+  __builtin_va_list ap;
+  __builtin_va_start(ap, a);
+  long double LD = __builtin_va_arg(ap, long double);
+  // GNU32-NOT: load x86_fp80*, x86_fp80**
+  // GNU32: load x86_fp80, x86_fp80*
+  // GNU64: load x86_fp80*, x86_fp80**
+  // GNU64: load x86_fp80, x86_fp80*
+  // MSC64-NOT: load double*, double**
+  // MSC64: load double, double*
+  __builtin_va_end(ap);
+}

clang/test/CodeGen/win64-i128.c

 // RUN: %clang_cc1 -triple x86_64-windows-gnu -emit-llvm -o - %s \
 // RUN:    | FileCheck %s --check-prefix=GNU64
 // RUN: %clang_cc1 -triple x86_64-windows-msvc -emit-llvm -o - %s \
 // RUN:    | FileCheck %s --check-prefix=MSC64
 
 typedef int int128_t __attribute__((mode(TI)));
 
 int128_t foo() { return 0; }
 
 // GNU64: define dso_local <2 x i64> @foo()
 // MSC64: define dso_local <2 x i64> @foo()
 
 int128_t bar(int128_t a, int128_t b) { return a * b; }
 
 // GNU64: define dso_local <2 x i64> @bar(i128* %0, i128* %1)
 // MSC64: define dso_local <2 x i64> @bar(i128* %0, i128* %1)
+
+void vararg(int a, ...) {
+  // GNU64-LABEL: define{{.*}} void @vararg
+  // MSC64-LABEL: define{{.*}} void @vararg
+  __builtin_va_list ap;
+  __builtin_va_start(ap, a);
+  int128_t i = __builtin_va_arg(ap, int128_t);
+  // GNU64: load i128*, i128**
+  // GNU64: load i128, i128*
+  // MSC64: load i128*, i128**
+  // MSC64: load i128, i128*
+  __builtin_va_end(ap);
+}

clang/test/CodeGenCXX/ext-int.cpp

void TakesVarargs(int i, ...) {
   // LIN: %[[GPOFFSET:.+]] = load i32, i32* %[[OFA_P1]]
   // LIN: %[[FITSINGP:.+]] = icmp ule i32 %[[GPOFFSET]], 32
   // LIN: br i1 %[[FITSINGP]]
   // LIN: %[[BC1:.+]] = phi i92*
   // LIN: %[[LOAD1:.+]] = load i92, i92* %[[BC1]]
   // LIN: store i92 %[[LOAD1]], i92*
 
   // WIN: %[[CUR1:.+]] = load i8*, i8** %[[ARGS]]
-  // WIN: %[[NEXT1:.+]] = getelementptr inbounds i8, i8* %[[CUR1]], i64 16
+  // WIN: %[[NEXT1:.+]] = getelementptr inbounds i8, i8* %[[CUR1]], i64 8
   // WIN: store i8* %[[NEXT1]], i8** %[[ARGS]]
-  // WIN: %[[BC1:.+]] = bitcast i8* %[[CUR1]] to i92*
-  // WIN: %[[LOADV1:.+]] = load i92, i92* %[[BC1]]
+  // WIN: %[[BC1:.+]] = bitcast i8* %[[CUR1]] to i92**
+  // WIN: %[[LOADP1:.+]] = load i92*, i92** %[[BC1]]
+  // WIN: %[[LOADV1:.+]] = load i92, i92* %[[LOADP1]]
   // WIN: store i92 %[[LOADV1]], i92*
 
   _ExtInt(31) B = __builtin_va_arg(args, _ExtInt(31));
   // LIN: %[[AD2:.+]] = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %[[ARGS]]
   // LIN: %[[OFA_P2:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD2]], i32 0, i32 0
   // LIN: %[[GPOFFSET:.+]] = load i32, i32* %[[OFA_P2]]
   // LIN: %[[FITSINGP:.+]] = icmp ule i32 %[[GPOFFSET]], 40
   // LIN: br i1 %[[FITSINGP]]
void TakesVarargs(int i, ...) {
   // LIN: %[[OFA4:.+]] = load i8*, i8** %[[OFA_P4]]
   // LIN: %[[BC4:.+]] = bitcast i8* %[[OFA4]] to i129*
   // LIN: %[[OFANEXT4:.+]] = getelementptr i8, i8* %[[OFA4]], i32 24
   // LIN: store i8* %[[OFANEXT4]], i8** %[[OFA_P4]]
   // LIN: %[[LOAD4:.+]] = load i129, i129* %[[BC4]]
   // LIN: store i129 %[[LOAD4]], i129*
 
   // WIN: %[[CUR4:.+]] = load i8*, i8** %[[ARGS]]
-  // WIN: %[[NEXT4:.+]] = getelementptr inbounds i8, i8* %[[CUR4]], i64 24
+  // WIN: %[[NEXT4:.+]] = getelementptr inbounds i8, i8* %[[CUR4]], i64 8
   // WIN: store i8* %[[NEXT4]], i8** %[[ARGS]]
-  // WIN: %[[BC4:.+]] = bitcast i8* %[[CUR4]] to i129*
-  // WIN: %[[LOADV4:.+]] = load i129, i129* %[[BC4]]
+  // WIN: %[[BC4:.+]] = bitcast i8* %[[CUR4]] to i129**
+  // WIN: %[[LOADP4:.+]] = load i129*, i129** %[[BC4]]
+  // WIN: %[[LOADV4:.+]] = load i129, i129* %[[LOADP4]]
   // WIN: store i129 %[[LOADV4]], i129*
 
   _ExtInt(16777200) E = __builtin_va_arg(args, _ExtInt(16777200));
   // LIN: %[[AD5:.+]] = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %[[ARGS]]
   // LIN: %[[OFA_P5:.+]] = getelementptr inbounds %struct.__va_list_tag, %struct.__va_list_tag* %[[AD5]], i32 0, i32 2
   // LIN: %[[OFA5:.+]] = load i8*, i8** %[[OFA_P5]]
   // LIN: %[[BC5:.+]] = bitcast i8* %[[OFA5]] to i16777200*
   // LIN: %[[OFANEXT5:.+]] = getelementptr i8, i8* %[[OFA5]], i32 2097152
   // LIN: store i8* %[[OFANEXT5]], i8** %[[OFA_P5]]
   // LIN: %[[LOAD5:.+]] = load i16777200, i16777200* %[[BC5]]
   // LIN: store i16777200 %[[LOAD5]], i16777200*
 
   // WIN: %[[CUR5:.+]] = load i8*, i8** %[[ARGS]]
-  // WIN: %[[NEXT5:.+]] = getelementptr inbounds i8, i8* %[[CUR5]], i64 2097152
+  // WIN: %[[NEXT5:.+]] = getelementptr inbounds i8, i8* %[[CUR5]], i64 8
   // WIN: store i8* %[[NEXT5]], i8** %[[ARGS]]
-  // WIN: %[[BC5:.+]] = bitcast i8* %[[CUR5]] to i16777200*
-  // WIN: %[[LOADV5:.+]] = load i16777200, i16777200* %[[BC5]]
+  // WIN: %[[BC5:.+]] = bitcast i8* %[[CUR5]] to i16777200**
+  // WIN: %[[LOADP5:.+]] = load i16777200*, i16777200** %[[BC5]]
+  // WIN: %[[LOADV5:.+]] = load i16777200, i16777200* %[[LOADP5]]
   // WIN: store i16777200 %[[LOADV5]], i16777200*
 
   __builtin_va_end(args);
   // LIN: %[[ENDAD:.+]] = getelementptr inbounds [1 x %struct.__va_list_tag], [1 x %struct.__va_list_tag]* %[[ARGS]]
   // LIN: %[[ENDAD1:.+]] = bitcast %struct.__va_list_tag* %[[ENDAD]] to i8*
   // LIN: call void @llvm.va_end(i8* %[[ENDAD1]])
   // WIN: %[[ARGSEND:.+]] = bitcast i8** %[[ARGS]] to i8*
   // WIN: call void @llvm.va_end(i8* %[[ARGSEND]])