[MS] Fix passing aligned records by value in some cases

[rnk]

Summary

It's not exactly clear what the meaning of TypeInfo::AlignRequirement
is, so go directly to the ASTRecordLayout for records and check the
required alignment there. Compare that number with the stack alignment
value of 4.

This fixes cases when the alignment attribute does not appear directly
on the record [1], or when the attribute on the record is underaligned
[2].

[1]: struct Foo { int __declspec(align(16)) x; };
[2]: struct __declspec(align(1)) Bar { int x; };

[nikic]

LGTM

[aaron.ballman]

Intel is seeing some fallout from this change in our downstream (reverting this commit causes our test case to pass). Our test looks like this:

#include <stdlib.h>
#include <stdio.h>
#include <stdarg.h>
#include <x86intrin.h>

typedef union {
	char c[32];
	short s[16];
	int d[8];
	long long l[4];
	__m128 h[2];
	__m256 m;
	__m256i mi;
	__m256d md;
} M256;

M256 make_m256_from_int(int d1, int d2, int d3, int d4, int d5, int d6, int d7, int d8) {
	M256 ret;
	ret.d[0] = d1;
	ret.d[1] = d2;
	ret.d[2] = d3;
	ret.d[3] = d4;
	ret.d[4] = d5;
	ret.d[5] = d6;
	ret.d[6] = d7;
	ret.d[7] = d8;
	return ret;
}

typedef union {
	char c;
} M8;

M8 make_m8_from_char(char c) {
	M8 ret;
	ret.c = c;
	return ret;
}

int test(char* format, ...) {
	int i, j;
	M256 m256;
	M8 m8;
	int retValue = 0;
	va_list args;
	va_start(args, format);

	for (i = 0; format[i]; i++) {
		switch (format[i]) {
			case '1':
				m8 = va_arg(args, M8);
				retValue += m8.c;
				break;
			case '6':
				m256 = va_arg(args, M256);
				retValue += m256.d[0] + m256.d[1] + m256.d[2] + m256.d[3] + m256.d[4] + m256.d[5] + m256.d[6] + m256.d[7];
				break;
		}
	}

	va_end(args);
	return retValue;
}

int main(void) {
	int retValue = 0;

	if (test("16", make_m8_from_char(-12), make_m256_from_int(1, 2, 1, 2, 1, 2, 1, 2))) {
		fprintf(stderr, "test failed for: M8(%d) M256(%d,%d,%d,%d,%d,%d,%d,%d)\n", 12, 1, 2, 1, 2, 1, 2, 1, 2);
		retValue++;
	}

	if (retValue) {
		printf("FAILED %d tests\n", retValue);
	} else {
		printf("PASSED\n");
	}
	return retValue;
}

What we're seeing is the difference in IR emitted by FE is the byval attribute and alignment for 3rd argument.

Bad

%call2 = call i32 (ptr, ...) @test(ptr noundef @"??_C@_02KMALDIDP@16?$AA@", ptr noundef byval(%union.M8) align 4 %agg.tmp1, ptr noundef %agg.tmp), !dbg !237

Good

%call2 = call i32 (ptr, ...) @test(ptr noundef @"??_C@_02KMALDIDP@16?$AA@", ptr noundef byval(%union.M8) align 4 %agg.tmp1, ptr noundef byval(%union.M256) align 4 %agg.tmp), !dbg !237

(We're seeing it with this clang-cl-esque command line: icx -c -Zi -Od /arch:AVX check_types_reduced.c -Xclang -emit-llvm -Xclang -disable-llvm-passes)

I didn't spot any UB in the test, so I think something may be incorrect with this patch (but codegen is not my area of specialty, so please correct me if I'm wrong).

[rnk]

Thanks, I think this is a clang bug. As I understand MSVC's behavior, we should not pass highly aligned variadic arguments indirectly: https://gcc.godbolt.org/z/Kr67xWTeE

I'll follow up on that.

[aaron.ballman]

In D152752#4611043, @rnk wrote:

Thanks, I think this is a clang bug. As I understand MSVC's behavior, we should not pass highly aligned variadic arguments indirectly: https://gcc.godbolt.org/z/Kr67xWTeE

I'll follow up on that.

Thank you!

[aaron.ballman]

In D152752#4611206, @aaron.ballman wrote:

In D152752#4611043, @rnk wrote:

Thanks, I think this is a clang bug. As I understand MSVC's behavior, we should not pass highly aligned variadic arguments indirectly: https://gcc.godbolt.org/z/Kr67xWTeE

I'll follow up on that.

Thank you!

Any update on a fix for this?

[rnk]

I need to get to it, my recollection is that @mstorsjo ran into the same issue here for mingw and made some changes, I wanted to go dig those up as a starting point. I may have completely forgotten things though.

[rnk]

I put together a fix here: https://github.com/llvm/llvm-project/compare/main...rnk:llvm-project:fix-vararg-align

I don't have arcanist set up like I used to, and I don't write as much code as I used to, so I kind of want to submit this as a pull request as soon as they are available this Thursday, if that's not an issue for you.

[mstorsjo]

In D152752#4626234, @rnk wrote:

I need to get to it, my recollection is that @mstorsjo ran into the same issue here for mingw and made some changes, I wanted to go dig those up as a starting point. I may have completely forgotten things though.

Hmm, I don't remember doing anything in that area - I don't think I've had to touch variadics on i386 (or x86_64 for that matter) so far, or anything relating to aligned variadics. (The main thing that might sound similar to alignment was about setting up the homed registers on aarch64 when receiving variadics; there's some special casing there relating to whether the number of homed registers is even or odd. But all of that is much deeper within lowering in LLVM.)

[rnk]

Following up, the vararg fix is here: https://github.com/llvm/llvm-project/pull/65692

[aaron.ballman]

In D152752#4641396, @rnk wrote:

Following up, the vararg fix is here: https://github.com/llvm/llvm-project/pull/65692

Thank you for the fix!

clang/lib/CodeGen/TargetInfo.cpp

if (shouldAggregateUseDirect(Ty, State, InReg, NeedsPadding)) {
       if (InReg)
         return ABIArgInfo::getDirectInReg(Result);
       else
         return ABIArgInfo::getDirect(Result);
     }
     llvm::IntegerType *PaddingType = NeedsPadding ? Int32 : nullptr;
 
     // Pass over-aligned aggregates on Windows indirectly. This behavior was
-    // added in MSVC 2015.
-    if (IsWin32StructABI && TI.isAlignRequired() && TI.Align > 32)
+    // added in MSVC 2015. Use the required alignment from the record layout,
+    // since that may be less than the regular type alignment, and types with
+    // required alignment of less than 4 bytes are not passed indirectly.
+    if (IsWin32StructABI) {
+      unsigned AlignInBits = 0;
+      if (RT) {
+        const ASTRecordLayout &Layout =
+          getContext().getASTRecordLayout(RT->getDecl());
+        AlignInBits = getContext().toBits(Layout.getRequiredAlignment());
+      } else if (TI.isAlignRequired()) {
+        AlignInBits = TI.Align;
+      }
+      if (AlignInBits > 32)
         return getIndirectResult(Ty, /*ByVal=*/false, State);
+    }
 
     // Expand small (<= 128-bit) record types when we know that the stack layout
     // of those arguments will match the struct. This is important because the
     // LLVM backend isn't smart enough to remove byval, which inhibits many
     // optimizations.
     // Don't do this for the MCU if there are still free integer registers
     // (see X86_64 ABI for full explanation).
     if (TI.Width <= 4 * 32 && (!IsMCUABI || State.FreeRegs == 0) &&

clang/test/CodeGen/X86/x86_32-arguments-win32.c

-// RUN: %clang_cc1 -w -triple i386-pc-win32 -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fms-extensions -w -triple i386-pc-win32 -emit-llvm -o - %s | FileCheck %s
 
 // CHECK-LABEL: define dso_local i64 @f1_1()
 // CHECK-LABEL: define dso_local void @f1_2(i32 %a0.0, i32 %a0.1)
 struct s1 {
   int a;
   int b;
 };
 struct s1 f1_1(void) { while (1) {} }

 // CHECK-LABEL: define dso_local void @pass_vec_128()
 // CHECK: call void @receive_vec_128(<4 x float> inreg noundef %{{[^,)]*}}, <4 x float> inreg noundef %{{[^,)]*}}, <4 x float> inreg noundef %{{[^,)]*}}, ptr noundef %{{[^,)]*}}, ptr noundef %{{[^,)]*}})
 
 
 void __fastcall fastcall_indirect_vec(__m128 x, __m128 y, __m128 z, __m128 w, int edx, __m128 q) {
   gv128 = x + y + z + w + q;
 }
 // CHECK-LABEL: define dso_local x86_fastcallcc void @"\01@fastcall_indirect_vec@84"(<4 x float> inreg noundef %x, <4 x float> inreg noundef %y, <4 x float> inreg noundef %z, ptr inreg noundef %0, i32 inreg noundef %edx, ptr noundef %1)
+
+struct __declspec(align(1)) Align1 { unsigned long long x; };
+struct __declspec(align(4)) Align4 { unsigned long long x; };
+struct __declspec(align(8)) Align8 { unsigned long long x; };
+void receive_align1(struct Align1 o);
+void receive_align4(struct Align4 o);
+void receive_align8(struct Align8 o);
+void pass_underaligned_record() {
+  struct Align1 a1;
+  receive_align1(a1);
+  struct Align4 a4;
+  receive_align4(a4);
+  struct Align8 a8;
+  receive_align8(a8);
+}
+// CHECK-LABEL: define dso_local void @pass_underaligned_record()
+// CHECK: call void @receive_align1(i64 {{[^,)]*}})
+// CHECK: call void @receive_align4(i64 {{[^,)]*}})
+// CHECK: call void @receive_align8(ptr {{[^,)]*}})
+
+struct FieldAlign1 { unsigned long long __declspec(align(1)) x; };
+struct FieldAlign4 { unsigned long long __declspec(align(4)) x; };
+struct FieldAlign8 { unsigned long long __declspec(align(8)) x; };
+void receive_falign1(struct FieldAlign1 o);
+void receive_falign4(struct FieldAlign4 o);
+void receive_falign8(struct FieldAlign8 o);
+void pass_underaligned_record_field() {
+  struct FieldAlign1 a1;
+  receive_falign1(a1);
+  struct FieldAlign4 a4;
+  receive_falign4(a4);
+  struct FieldAlign8 a8;
+  receive_falign8(a8);
+}
+// CHECK-LABEL: define dso_local void @pass_underaligned_record_field()
+// CHECK: call void @receive_falign1(i64 {{[^,)]*}})
+// CHECK: call void @receive_falign4(i64 {{[^,)]*}})
+// CHECK: call void @receive_falign8(ptr {{[^,)]*}})