x86-64 ABI: unwrap single element structs / arrays of 256-bit vectors to pass and return in registers

[spatel]

Summary

This is a patch for PR22563 ( http://llvm.org/bugs/show_bug.cgi?id=22563 ).

We were not correctly unwrapping a single 256-bit AVX vector that was defined as an array of 1 inside a struct.

We would generate a <4 x float> param/return value instead of <8 x float> and lose half of the vector.

[hfinkel]

Indeed, this looks like an odd mismatch between X86_64ABIInfo::GetByteVectorType and the recursive logic in X86_64ABIInfo::classify (which will recurse and handle the array types).

FWIW, I wonder if there are more bugs hiding in here too. When X86_64ABIInfo::classify recurses on the array types, it looks like it is just looking at the total size of the array. isSingleElementStruct specifically handles only arrays of 1 element. What happens when you try to pass a struct of { v4f32 x[2]; }?

If that works, then LGTM (otherwise, we obviously need to fix that too).

[spatel]

In D7614#124120, @hfinkel wrote:

What happens when you try to pass a struct of { v4f32 x[2]; }?

Great question. There's a check in classify() around here:
https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/TargetInfo.cpp#L2044

// 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.
//
if (Size > 128 && getContext().getTypeSize(i->getType()) != 256) {

And so we don't try to pass/return anything in registers with an array of two 128-bit vectors.

And this also triggers on the even simpler case of:
struct v4f32_wrapper {

v4f32 v1;
v4f32 v2;

};

Based on my reading of the ABI, I would've thought that could be passed as 2 xmm registers, but we pass by memory for this too. FWIW, gcc 4.9 and icc 15 do the same in these cases, so I think this is just the way things are supposed to be.

[hfinkel]

In D7614#124323, @spatel wrote:

In D7614#124120, @hfinkel wrote:

What happens when you try to pass a struct of { v4f32 x[2]; }?

Great question. There's a check in classify() around here:
https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/TargetInfo.cpp#L2044

// 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.
//
if (Size > 128 && getContext().getTypeSize(i->getType()) != 256) {

And so we don't try to pass/return anything in registers with an array of two 128-bit vectors.

And this also triggers on the even simpler case of:
struct v4f32_wrapper {

v4f32 v1;
v4f32 v2;

};

Based on my reading of the ABI, I would've thought that could be passed as 2 xmm registers, but we pass by memory for this too. FWIW, gcc 4.9 and icc 15 do the same in these cases, so I think this is just the way things are supposed to be.

Okay, great. Please commit.

Also, if the ABI spec differs from implementation practice, we should also send a note to whomever maintains that and try to get it clarified.

cfe/trunk/lib/CodeGen/TargetInfo.cpp

if (freeIntRegs == 0) {
     if (Align == 8 && Size <= 64)
       return ABIArgInfo::getDirect(llvm::IntegerType::get(getVMContext(),
                                                           Size));
   }
 
   return ABIArgInfo::getIndirect(Align);
 }
 
-/// GetByteVectorType - The ABI specifies that a value should be passed in an
-/// full vector XMM/YMM register.  Pick an LLVM IR type that will be passed as a
-/// vector register.
+/// The ABI specifies that a value should be passed in a full vector XMM/YMM
+/// register. Pick an LLVM IR type that will be passed as a vector register.
 llvm::Type *X86_64ABIInfo::GetByteVectorType(QualType Ty) const {
-  llvm::Type *IRType = CGT.ConvertType(Ty);
+  // 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);
 
-  // Wrapper structs that just contain vectors are passed just like vectors,
-  // strip them off if present.
-  llvm::StructType *STy = dyn_cast<llvm::StructType>(IRType);
-  while (STy && STy->getNumElements() == 1) {
-    IRType = STy->getElementType(0);
-    STy = dyn_cast<llvm::StructType>(IRType);
-  }
+  llvm::Type *IRType = CGT.ConvertType(Ty);
 
   // If the preferred type is a 16-byte vector, prefer to pass it.
   if (llvm::VectorType *VT = dyn_cast<llvm::VectorType>(IRType)){
     llvm::Type *EltTy = VT->getElementType();
     unsigned BitWidth = VT->getBitWidth();
     if ((BitWidth >= 128 && BitWidth <= 256) &&
         (EltTy->isFloatTy() || EltTy->isDoubleTy() ||
          EltTy->isIntegerTy(8) || EltTy->isIntegerTy(16) ||

cfe/trunk/test/CodeGen/x86_64-arguments.c

struct v4f32wrapper {
   v4f32 v;
 };
 
 struct v4f32wrapper f27(struct v4f32wrapper X) {
   // CHECK-LABEL: define <4 x float> @f27(<4 x float> %X.coerce)
   return X;
 }
 
+// PR22563 - We should unwrap simple structs and arrays to pass
+// and return them in the appropriate vector registers if possible.
+
+typedef float v8f32 __attribute__((__vector_size__(32)));
+struct v8f32wrapper {
+  v8f32 v;
+};
+
+struct v8f32wrapper f27a(struct v8f32wrapper X) {
+  // AVX-LABEL: define <8 x float> @f27a(<8 x float> %X.coerce)
+  return X;
+}
+
+struct v8f32wrapper_wrapper {
+  v8f32 v[1];
+};
+
+struct v8f32wrapper_wrapper f27b(struct v8f32wrapper_wrapper X) {
+  // AVX-LABEL: define <8 x float> @f27b(<8 x float> %X.coerce)
+  return X;
+}
+
 // rdar://5711709
 struct f28c {
   double x;
   int y;
 };
 void f28(struct f28c C) {
   // CHECK-LABEL: define void @f28(double %C.coerce0, i32 %C.coerce1)
 }