[X86] Prevent passing vectors of __int128 as <X x i128> in llvm IR

[craig.topper]

Summary

As far as I can tell, gcc passes 256/512 bit vectors __int128 in memory. And passes a vector of 1 _int128 in an xmm register. The backend considers <X x i128> as an illegal type and will scalarize any arguments with that type. So we need to coerce the argument types in the frontend to match to avoid the illegal type.

Are there other element types to consider? Do we need to keep the old behavior on platforms where clang is the de facto compiler?

This issue was identified in PR42607. Though even with the types changed, we still seem to be doing some unnecessary stack realignment.

I'll add test cases later today or over the weekend.

[wristow]

Do we need to keep the old behavior on platforms where clang is the de facto compiler?

I know we (PlayStation) will want to keep the old behavior.

[xbolva00]

Maybe this ABI issue should be fixed for 9.0 ? Not sure how important, but
@hans maybe should track this one.

[craig.topper]

Add test cases. Change the type to vXi64 instead of vXf64. Add abi compatibility flag. Restrict to linux and freebsd.

[craig.topper]

More test check lines. Test the compatibility flag.

[hans]

In D64672#1623784, @xbolva00 wrote:

Maybe this ABI issue should be fixed for 9.0 ? Not sure how important, but
@hans maybe should track this one.

IIUC, this is not a new issue, so I'd prefer not trying to rush it in for llvm 9, and rather get it right for llvm 10 instead.

[RKSimon]

Please can you extend the test coverage to cover more vector sizes and cpu target features, not just avx512, and also add x86_64-scei-ps4 triple tests. Maybe 32-bit tests as well?

clang/include/clang/Basic/LangOptions.h

143

Add that this is just for Linux and NetBSD

[craig.topper]

In D64672#1653847, @RKSimon wrote:

Please can you extend the test coverage to cover more vector sizes and cpu target features, not just avx512, and also add x86_64-scei-ps4 triple tests. Maybe 32-bit tests as well?

Other than 256-bit what other size do you want?

[craig.topper]

Add more tests. Clarify which platforms are affected in LangOptions.h

[RKSimon]

Tests look great - please can you pre-commit them and update the patch to show the diff? Also, maybe call the test file x86-vec-i128.c and add a comment in the file describing PR42607?

[craig.topper]

In D64672#1654687, @RKSimon wrote:

Tests look great - please can you pre-commit them and update the patch to show the diff? Also, maybe call the test file x86-vec-i128.c and add a comment in the file describing PR42607?

How do you want me to show the diff? There a couple options. Three of the check lines have to be removed from the pre-commit since they use a command line option that doesn't exist. Do you want me to change the check lines that say NEWABI to use the old code? Or do you want me to manipulate the FileCheck command lines to use the OLDABI check lines?

[RKSimon]

In D64672#1654983, @craig.topper wrote:

In D64672#1654687, @RKSimon wrote:

Tests look great - please can you pre-commit them and update the patch to show the diff? Also, maybe call the test file x86-vec-i128.c and add a comment in the file describing PR42607?

How do you want me to show the diff? There a couple options. Three of the check lines have to be removed from the pre-commit since they use a command line option that doesn't exist. Do you want me to change the check lines that say NEWABI to use the old code? Or do you want me to manipulate the FileCheck command lines to use the OLDABI check lines?

How about CLANG9ABI and CLANG10ABI? And the patch just adds the -fclang-abi-compat=9 tests?

[craig.topper]

Change test name and adjust to the changes I think @RKSimon was asking for. I haven't commited the test yet. I'd like to verify that the before version of the test is how you would like it pre-committed

[probinson]

It looks to me like the patch does things the New Way only for Linux and NetBSD, so for PS4 backward compatibility I am okay with it.

[RKSimon]

LGTM - cheers

[RKSimon]

@craig.topper Wasn't this committed at rL371169 ?

[craig.topper]

Committed in r371169, but I forgot the Differential Revision line

clang/docs/ReleaseNotes.rst

 OpenCL C Language Changes in Clang
 ----------------------------------
 
 ...
 
 ABI Changes in Clang
 --------------------
 
-- ...
+- gcc passes vectors of __int128 in memory on X86-64. Clang historically
+  broke the vectors into multiple scalars using two 64-bit values for each
+  element. Clang now matches the gcc behavior on Linux and NetBSD. You can
+  switch back to old API behavior with flag: -fclang-abi-compat=9.0.
 
 OpenMP Support in Clang
 -----------------------
 
 - ...
 
 CUDA Support in Clang
 ---------------------

clang/include/clang/Basic/LangOptions.h

enum class ClangABI {
     Ver6,
 
     /// Attempt to be ABI-compatible with code generated by Clang 7.0.x
     /// (SVN r338536). This causes alignof (C++) and _Alignof (C11) to be
     /// compatible with __alignof (i.e., return the preferred alignment)
     /// rather than returning the required alignment.
     Ver7,
 
+    /// Attempt to be ABI-compatible with code generated by Clang 9.0.x
+    /// (SVN r351319). This causes vectors of __int128 to be passed in memory
+    /// instead of passing in multiple scalar registers.
+    Ver9,
+
     /// Conform to the underlying platform's C and C++ ABIs as closely
     /// as we can.
     Latest
   };
 
   enum class CoreFoundationABI {
     /// No interoperability ABI has been specified
     Unspecified,

clang/lib/CodeGen/TargetInfo.cpp

bool classifyIntegerMMXAsSSE() const {
     const llvm::Triple &Triple = getTarget().getTriple();
     if (Triple.isOSDarwin() || Triple.getOS() == llvm::Triple::PS4)
       return false;
     if (Triple.isOSFreeBSD() && Triple.getOSMajorVersion() >= 10)
       return false;
     return true;
   }
 
+  // GCC classifies vectors of __int128 as memory.
+  bool passInt128VectorsInMem() const {
+    // Clang <= 9.0 did not do this.
+    if (getContext().getLangOpts().getClangABICompat() <=
+        LangOptions::ClangABI::Ver9)
+      return false;
+
+    const llvm::Triple &T = getTarget().getTriple();
+    return T.isOSLinux() || T.isOSNetBSD();
+  }
+
   X86AVXABILevel AVXLevel;
   // Some ABIs (e.g. X32 ABI and Native Client OS) use 32 bit pointers on
   // 64-bit hardware.
   bool Has64BitPointers;
 
 public:
   X86_64ABIInfo(CodeGen::CodeGenTypes &CGT, X86AVXABILevel AVXLevel) :
       SwiftABIInfo(CGT), AVXLevel(AVXLevel),
if (Size == 1 || Size == 8 || Size == 16 || Size == 32) {
         Current = SSE;
 
       // If this type crosses an eightbyte boundary, it should be
       // split.
       if (OffsetBase && OffsetBase != 64)
         Hi = Lo;
     } else if (Size == 128 ||
                (isNamedArg && Size <= getNativeVectorSizeForAVXABI(AVXLevel))) {
+      QualType ElementType = VT->getElementType();
+
+      // gcc passes 256 and 512 bit <X x __int128> vectors in memory. :(
+      if (passInt128VectorsInMem() && Size != 128 &&
+          (ElementType->isSpecificBuiltinType(BuiltinType::Int128) ||
+           ElementType->isSpecificBuiltinType(BuiltinType::UInt128)))
+        return;
+
       // Arguments of 256-bits are split into four eightbyte chunks. The
       // least significant one belongs to class SSE and all the others to class
       // SSEUP. The original Lo and Hi design considers that types can't be
       // greater than 128-bits, so a 64-bit split in Hi and Lo makes sense.
       // This design isn't correct for 256-bits, but since there're no cases
       // where the upper parts would need to be inspected, avoid adding
       // complexity and just consider Hi to match the 64-256 part.
       //

 }
 
 bool X86_64ABIInfo::IsIllegalVectorType(QualType Ty) const {
   if (const VectorType *VecTy = Ty->getAs<VectorType>()) {
     uint64_t Size = getContext().getTypeSize(VecTy);
     unsigned LargestVector = getNativeVectorSizeForAVXABI(AVXLevel);
     if (Size <= 64 || Size > LargestVector)
       return true;
+    QualType EltTy = VecTy->getElementType();
+    if (passInt128VectorsInMem() &&
+        (EltTy->isSpecificBuiltinType(BuiltinType::Int128) ||
+         EltTy->isSpecificBuiltinType(BuiltinType::UInt128)))
+      return true;
   }
 
   return false;
 }
 
 ABIArgInfo X86_64ABIInfo::getIndirectResult(QualType Ty,
                                             unsigned freeIntRegs) const {
   // If this is a scalar LLVM value then assume LLVM will pass it in the right

 /// 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);
-  if (isa<llvm::VectorType>(IRType) ||
-      IRType->getTypeID() == llvm::Type::FP128TyID)
+  if (isa<llvm::VectorType>(IRType)) {
+    // Don't pass vXi128 vectors in their native type, the backend can't
+    // legalize them.
+    if (passInt128VectorsInMem() &&
+        IRType->getVectorElementType()->isIntegerTy(128)) {
+      // Use a vXi64 vector.
+      uint64_t Size = getContext().getTypeSize(Ty);
+      return llvm::VectorType::get(llvm::Type::getInt64Ty(getVMContext()),
+                                   Size / 64);
+    }
+
+    return IRType;
+  }
+
+  if (IRType->getTypeID() == llvm::Type::FP128TyID)
     return IRType;
 
   // We couldn't find the preferred IR vector type for 'Ty'.
   uint64_t Size = getContext().getTypeSize(Ty);
   assert((Size == 128 || Size == 256 || Size == 512) && "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

clang/lib/Frontend/CompilerInvocation.cpp

if (!VerParts.first.startswith("0") &&
       if (Major == 3 && Minor <= 8)
         Opts.setClangABICompat(LangOptions::ClangABI::Ver3_8);
       else if (Major <= 4)
         Opts.setClangABICompat(LangOptions::ClangABI::Ver4);
       else if (Major <= 6)
         Opts.setClangABICompat(LangOptions::ClangABI::Ver6);
       else if (Major <= 7)
         Opts.setClangABICompat(LangOptions::ClangABI::Ver7);
+      else if (Major <= 9)
+        Opts.setClangABICompat(LangOptions::ClangABI::Ver9);
     } else if (Ver != "latest") {
       Diags.Report(diag::err_drv_invalid_value)
           << A->getAsString(Args) << A->getValue();
     }
   }
 
   Opts.CompleteMemberPointers = Args.hasArg(OPT_fcomplete_member_pointers);
   Opts.BuildingPCHWithObjectFile = Args.hasArg(OPT_building_pch_with_obj);

clang/test/CodeGen/pr42607.c

+// RUN: %clang_cc1 -triple x86_64-linux-gnu %s -target-feature +avx512f -S -emit-llvm -o - | FileCheck %s --check-prefix=NEWABI
+// RUN: %clang_cc1 -triple x86_64-apple-darwin %s -target-feature +avx512f -S -emit-llvm -o - | FileCheck %s --check-prefix=OLDABI
+// RUN: %clang_cc1 -triple x86_64-unknown-freebsd10.0 %s -target-feature +avx512f -S -emit-llvm -o - | FileCheck %s --check-prefix=OLDABI
+// RUN: %clang_cc1 -triple x86_64-linux-gnu %s -target-feature +avx512f -S -emit-llvm -o - -fclang-abi-compat=9 | FileCheck %s --check-prefix=OLDABI
+
+typedef unsigned long long v16u64 __attribute__((vector_size(16)));
+typedef unsigned __int128 v16u128 __attribute__((vector_size(16)));
+
+v16u64 bar(v16u64 a, v16u128 b) {
+// NEWABI: define <2 x i64> @bar(<2 x i64> %{{.*}}, <2 x i64> %{{.*}})
+// OLDABI: define <2 x i64> @bar(<2 x i64> %{{.*}}, <1 x i128> %{{.*}})
+  return a + (v16u64)b;
+}
+
+typedef unsigned long long v64u64 __attribute__((vector_size(64)));
+typedef unsigned __int128 v64u128 __attribute__((vector_size(64)));
+
+v64u64 foo(v64u64 a, v64u128 b) {
+// NEWABI: define <8 x i64> @foo(<8 x i64> %{{.*}}, <4 x i128>* byval(<4 x i128>) align 64 %{{.*}})
+// OLDABI: define <8 x i64> @foo(<8 x i64> %{{.*}}, <4 x i128> %{{.*}})
+  return a + (v64u64)b;
+}