[X86] Passing union type through register.

[LiuChen3]

Summary

For example:

union M256 {
  double d;
  __m256 m;
};

extern void foo1(union M256 A);

union M256 m1;

void test() {
  foo1(m1);
}

clang will pass it through stack because because the judgement: Size != getContext().getTypeSize(i->getType()) will return true.
This is not correct for union type.

Also, I think there are some problems on comments. I did some change.

[craig.topper]

clang/lib/CodeGen/TargetInfo.cpp

2886

Is there not a isUnion() or isUnionType()?

[LiuChen3]

Address the comment.

[LiuChen3]

clang/lib/CodeGen/TargetInfo.cpp

2886

Yes. Thanks for the advise.

[rjmccall]

clang/lib/CodeGen/TargetInfo.cpp

2910

This change seems correct, but I would be a lot happier if we just extended the Lo/Hi logic properly. The current code is making a lot of questionable assumptions about struct layout.

[LiuChen3]

clang/lib/CodeGen/TargetInfo.cpp

2910

I will work on it, later.

[LiuChen3]

clang/lib/CodeGen/TargetInfo.cpp

2910

Hi, @rjmccall.
I have checked the logic here. I think the we extended the Lo/Hi logic properly if we exclude union type. For union type, I think we should assume that all elements are the same size as union itself. So the class of any size of union type can be determined like the union which size no bigger than 128. What's your concern here, maybe you can give me an example?

[LiuChen3]

clang/lib/CodeGen/TargetInfo.cpp

2910

I think at least the method of determining the number of elements by judging whether the element size and the aggregate size are equal is wrong for union type. I am afraid that extending the current Lo/Hi logic will cause many failures.

[LiuChen3]

Ping?

[LiuChen3]

Hi, @majnemer, @bruno. I saw that both of you have modified this logic. What's your opinion here?

[bruno]

Hi, @majnemer, @bruno. I saw that both of you have modified this logic. What's your opinion here?

It has been a long time to remember details, but I share @rjmccall concerns. @craig.topper wdyt?

clang/test/CodeGen/passing-union.c

2

You probably don't need -fblocks here.

6

Instead of this include can you just define __m256 and __m512?

[LiuChen3]

Address comments

[LiuChen3]

It has been a long time to remember details, but I share @rjmccall concerns.

Thanks, @bruno. Modifying the existing logic is risky. If we have to do that, I think a new patch will be better. The purpose of this patch is to solve an ABI bug.

[LiuChen3]

Include the context.

[bruno]

Thanks, @bruno. Modifying the existing logic is risky. If we have to do that, I think a new patch will be better. The purpose of this patch is to solve an ABI bug.

Ok, I think it's reasonable that you get to fix this bug without having to redesign the Lo/Hi logic here. Two more things:

• Can you rewrite part of the comment and add a more explicit FIXME for the Lo/Hi stuff?
• Place the test in clang/test/CodeGen/X86 and rename it to avx-union.c

[LiuChen3]

Address comments.

[bruno]

LGTM

[LiuChen3]

In D78699#2320678, @bruno wrote:

LGTM

Thanks for your review.

[dyung]

Hi, the test you added seems to pass both before and after your change, is this intended?

[dyung]

clang/test/CodeGen/X86/avx-union.c

2 ↗

(On Diff #297115)

These RUN lines have 2 issues, you are using "||" instead of "|" and each check prefix must have its own argument, so you need "--check-prefix=CHECK --check-prefix=AVX".

22 ↗

(On Diff #297115)

When I fix the run lines above, I'm not getting "dso_local". Might want to double check that.

[LiuChen3]

In D78699#2328541, @dyung wrote:

Hi, the test you added seems to pass both before and after your change, is this intended?

Oh, yes. I made mistake on my RUN line. So these tests are not really checked. Thanks for your help!
I will make another patch to fix this testcase.

clang/test/CodeGen/X86/avx-union.c

22 ↗

(On Diff #297115)

There actually have no 'dso_local'.

clang/lib/CodeGen/TargetInfo.cpp

if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
           postMerge(Size, Lo, Hi);
           return;
         }
       }
     }
 
     // Classify the fields one at a time, merging the results.
     unsigned idx = 0;
+    const RecordType *IsUnion = RT->getAsUnionType();
     for (RecordDecl::field_iterator i = RD->field_begin(), e = RD->field_end();
            i != e; ++i, ++idx) {
       uint64_t Offset = OffsetBase + Layout.getFieldOffset(idx);
       bool BitField = i->isBitField();
 
       // Ignore padding bit-fields.
       if (BitField && i->isUnnamedBitfield())
         continue;
 
       // AMD64-ABI 3.2.3p2: Rule 1. If the size of an object is larger than
-      // four eightbytes, or it contains unaligned fields, it has class MEMORY.
+      // eight eightbytes, or it contains unaligned fields, it has class MEMORY.
       //
-      // The only case a 256-bit wide vector could be used is when the struct
-      // contains a single 256-bit element. Since Lo and Hi logic isn't extended
-      // to work for sizes wider than 128, early check and fallback to memory.
+      // The cases a 256-bit or a 512-bit wide vector could be used is when the
+      // struct contains a single 256-bit or 512-bit element. Since Lo and Hi
+      // logic isn't extended to work for sizes wider than 128, early check and
+      // fallback to memory.
       //
-      if (Size > 128 && (Size != getContext().getTypeSize(i->getType()) ||
-                         Size > getNativeVectorSizeForAVXABI(AVXLevel))) {
+      if (Size > 128 &&
+          ((!IsUnion && Size != getContext().getTypeSize(i->getType())) ||
         Lo = Memory;
         postMerge(Size, Lo, Hi);
         return;
       }
       // Note, skip this test for bit-fields, see below.
       if (!BitField && Offset % getContext().getTypeAlign(i->getType())) {
         Lo = Memory;
         postMerge(Size, Lo, Hi);
         return;
       }
 
       // Classify this field.

clang/test/CodeGen/passing-union.c

+// RUN: %clang_cc1 -w -fblocks -ffreestanding -triple x86_64-linux-gnu -target-feature +avx -emit-llvm -o %t %s || FileCheck < %t %s --check-prefix=CHECK, AVX
+// RUN: %clang_cc1 -w -fblocks -ffreestanding -triple x86_64-linux-gnu -target-feature +avx512f -emit-llvm -o %t %s || FileCheck < %t %s --check-prefix=CHECK, AVX512
+// This test verifies that passing union parameters through register.
+
+#include <immintrin.h>
+
+union M256 {
+  double d;
+  __m256 m;
+};
+
+union M512 {
+  double d;
+  __m512 m;
+};
+
+extern void foo1(union M256 A);
+extern void foo2(union M512 A);
+union M256 m1;
+union M512 m2;
+// CHECK-LABEL: define dso_local void @test()
+// CHECK:       void @foo1(<4 x double>
+// AVX:         call void @foo2(%union.M512* byval(%union.M512) align 64
+// AVX512:      call void @foo2(<8 x double>
+void test() {
+  foo1(m1);
+  foo2(m2);
+}