[x86-64 ABI] Fix for PR23082: an assertion failure when passing/returning a wrapper union in a full YMM register.

[andreadb]

Summary

This patch fixes PR23082.

For the purpose of ABI classification, vectors are classified based on their 'Width'. The 'Width' is computed starting from the size of the element type multiplied by the number of packed elements in the vector. However, if the 'Width' is not a power-of-2, it gets implicitly rounded to the next power-of-2.

By construction, ABI class "SSEUp" is only used for vector types (or aggregate data structures that wrap vector type).
Also, by construction, the Width of a vector type of ABI class 'SSEUp' can only be 16 or 32 bytes.

So, when method 'GetByteVectorType' is called, the size of type 'Ty' can only be either 16 or 32 bytes. At this point, the Frontend could just return an IR vector type that matches in size the size of QualType 'Ty'. For example, the Frontend could return <2 x double> for the case where the Size is 128 bits (this used to be the default before r229408); alternatively, a vector type <4 x double> for the case where Size is 256 bits).

What currently happens is that the Frontend tries to be friendly and select the "best" IR vector type for the QualType 'Ty'. In particular, if 'Ty' is a wrapper structure, it keeps unwrapping it until it finds a vector type VTy. That VTy would then be our "preferred IR type". However, method 'isSingleElementStructure' (which is used to unwrap structures) doesn't know how to 'look through unions'. So, if end up with a nesting of wrapper structs/union we might end up triggering the assertion added at revision 230971.

In my experiments, the assertion failure reported as PR23082 only occurs if we a nest of wrapper structures with at least two union types. In all other cases, calling method 'isSingleElementStructure' is always okay.

I decided to address the problem in a simple way:
if we fail to find the "preferred type" for a single element structure 'Ty', then we just return a potentially "less friendly" vector type which would still be valid according to the ABI. So, rather than asserting on a valid type in input, we return a vector type which is <2 x double> if size is 16 bytes, or <4 x double> if size is 32 bytes.
An alternative approach consists in teaching 'isSingleElementStructure' how to handle unions. However, I decided to go for the safest approach (since 'isSingleElementStructure' is also used by other ABI, and not only by the x86-64 ABI) and conservatively return a vector type which is always known to be okay for the ABI.

Please let me know what you think.

Thanks,
Andrea

[bkramer]

LGTM, thanks!

[andreadb]

Thanks Benjamin!
Committed revision 238861.

cfe/trunk/lib/CodeGen/TargetInfo.cpp

 /// register. Pick an LLVM IR type that will be passed as a vector register.
 llvm::Type *X86_64ABIInfo::GetByteVectorType(QualType Ty) const {
   // Wrapper structs/arrays that only contain vectors are passed just like
   // vectors; strip them off if present.
   if (const Type *InnerTy = isSingleElementStruct(Ty, getContext()))
     Ty = QualType(InnerTy, 0);
 
   llvm::Type *IRType = CGT.ConvertType(Ty);
-  assert(isa<llvm::VectorType>(IRType) &&
-         "Trying to return a non-vector type in a vector register!");
+  if(isa<llvm::VectorType>(IRType))
     return IRType;
+
+  // We couldn't find the preferred IR vector type for 'Ty'.
+  uint64_t Size = getContext().getTypeSize(Ty);
+  assert((Size == 128 || Size == 256) && "Invalid type found!");
+
+  // Return a LLVM IR vector type based on the size of 'Ty'.
+  return llvm::VectorType::get(llvm::Type::getDoubleTy(getVMContext()),
+                               Size / 64);
 }
 
 /// BitsContainNoUserData - Return true if the specified [start,end) bit range
 /// is known to either be off the end of the specified type or being in
 /// alignment padding.  The user type specified is known to be at most 128 bits
 /// in size, and have passed through X86_64ABIInfo::classify with a successful
 /// classification that put one of the two halves in the INTEGER class.
 ///

cfe/trunk/test/CodeGenCXX/x86_64-arguments-avx.cpp

 // RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s -target-feature +avx | FileCheck %s
 
 namespace test1 {
 typedef double __m256d __attribute__((__vector_size__(32)));
 
 class PR22753 {
 public:
   __m256d data;
 };
 
 // CHECK: define <4 x double> @_ZN5test14testENS_7PR22753E(<4 x double>
 PR22753 test(PR22753 x) {
   return x;
 }
 }
+
+namespace test2 {
+typedef double __m128d __attribute__((__vector_size__(16)));
+typedef float __m128 __attribute__((__vector_size__(16)));
+typedef double __m256d __attribute__((__vector_size__(32)));
+typedef float __m256 __attribute__((__vector_size__(32)));
+
+union U1 {
+  __m128  v1;
+  __m128d v2;
+};
+
+union UU1 {
+  union U1;
+  __m128d v3;
+};
+
+// CHECK: define <2 x double> @_ZN5test27PR23082ENS_3UU1E(<2 x double>
+UU1 PR23082(UU1 x) {
+  return x;
+}
+
+union U2 {
+  __m256  v1;
+  __m256d v2;
+};
+
+union UU2 {
+  union U2;
+  __m256d v3;
+};
+
+// CHECK: define <4 x double> @_ZN5test27PR23082ENS_3UU2E(<4 x double>
+UU2 PR23082(UU2 x) {
+  return x;
+}
+}