[CodeGen] Teach X86_64ABIInfo about AVX512.

[ab]

Summary

Tentative but straightforward patch, based on the "Edited for AVX512" SysV ABI drafts.

Depends on a local refactoring to pass the ABI string rather than HasAVX.

[ab]

lib/CodeGen/TargetInfo.cpp

1634–1635

This I'm not sure; it makes sense to me to align to 64 when we have AVX512.

There's also the WinX86_64 counterpart, which I'm even less confident about.

[rjmccall]

lib/CodeGen/TargetInfo.cpp

1634–1635

I am pretty skeptical of even the original code here, frankly. Are clients really making stronger promises just because they're compiling with AVX enabled? Note that the compiler doesn't *do* anything to achieve this; it literally just makes a more aggressive assumption about what the user has done.

It seems to me like the (1) the OpenMP people ought to review this and seriously consider it and (2) this number needs to be available to code in some portable manner, probably via a predefined macro. Which means it probably ought to be defined in the AST-level TargetInfo. CC'ing Alexey Bataev.

1934

Please parenthesize this second ||. It's actually wrong as written: && binds tighter than ||, but you want isNamedArg to apply to both.

[ABataev]

lib/CodeGen/TargetInfo.cpp

1634–1635

John, here is an excerpt from the OpenMP "If no optional parameter is specified, implementation-defined default
alignments for SIMD instructions on the target platforms are assumed." So, the value can be chosen by the compiler. But I think it is a good idea to have a macro for this.

[rjmccall]

lib/CodeGen/TargetInfo.cpp

1634–1635

Okay. Would you mind making sure that happens? In the meantime, I think we should bump the alignment up to 64 here when AVX512 is enabled.

[ABataev]

No problems

Best regards,

Alexey Bataev

Software Engineer
Intel Compiler Team

21.05.2015 19:48, John McCall пишет:

Comment at: lib/CodeGen/TargetInfo.cpp:1634-1635
@@ -1630,3 +1633,4 @@

unsigned getOpenMPSimdDefaultAlignment(QualType) const override {

+ // FIXME: What about AVX512?

  return getABIInfo().hasAVX() ? 32 : 16;
}

---

ABataev wrote:

rjmccall wrote:

ab wrote:

This I'm not sure; it makes sense to me to align to 64 when we have AVX512.

There's also the WinX86_64 counterpart, which I'm even less confident about.

I am pretty skeptical of even the original code here, frankly. Are clients really making stronger promises just because they're compiling with AVX enabled? Note that the compiler doesn't *do* anything to achieve this; it literally just makes a more aggressive assumption about what the user has done.

It seems to me like the (1) the OpenMP people ought to review this and seriously consider it and (2) this number needs to be available to code in some portable manner, probably via a predefined macro. Which means it probably ought to be defined in the AST-level TargetInfo. CC'ing Alexey Bataev.

John, here is an excerpt from the OpenMP "If no optional parameter is specified, implementation-defined default
alignments for SIMD instructions on the target platforms are assumed." So, the value can be chosen by the compiler. But I think it is a good idea to have a macro for this.

Okay. Would you mind making sure that happens? In the meantime, I think we should bump the alignment up to 64 here when AVX512 is enabled.

http://reviews.llvm.org/D9894

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/

[ab]

• Add OpenMP default alignment for AVX512
• Extract HasAVX/HasAVX512 into a X86_64BaseABIInfo

In r237989 I refactored the "HasAVX" flag to just use the TargetInfo::getABI() string, but I'm tempted to revert, and pass down an additional HasAVX512 flag. I originally wanted to do the same thing for other targets, but didn't realize that some of the ABIs depend on things not in TargetInfo, e.g. CodeGenOptions, which aren't supposed to be available?

Anyway, opinions welcome, one way or the other.

[ab]

Ping!

[rjmccall]

I like the idea of passing it down instead of working off of strings. Maybe pass down an enum instead of two flags?

[ABataev]

Hi John,
tried to add a macro for default alignment, but it does not work. Method
getOpenMPSimdDefaultAlignment() has a parameter QualType, which used for
some specific targets and makes this method to return different default
alignment for different types on some platforms (for example, for
PPC64_SVR4. So you can't just create a single macro with the predefined
value that could be used for all types.

Best regards,

Alexey Bataev

Software Engineer
Intel Compiler Team

18.06.2015 10:25, John McCall пишет:

I like the idea of passing it down instead of working off of strings. Maybe pass down an enum instead of two flags?

http://reviews.llvm.org/D9894

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/

[rjmccall]

Okay, that gives us two options. The first is to use a macro that's conservative for all types; the second is to add a new type trait. If the performance difference on PPC64 is important, the type trait seems fine to me.

I guess it would have to be a UnaryExprOrTypeTrait, because the unary type traits don't currently allow returning size_t.

[ab]

• Use an AVXLevel enum
• Deduplicate AVXLevel -> size pattern

With that, adding AVX512 support is a trivial 4 line patch.

[rjmccall]

LGTM.

lib/Basic/Targets.cpp

public:
   // getDefaultFeatures which calls this repeatedly.
   static void setFeatureEnabledImpl(llvm::StringMap<bool> &Features,
                                     StringRef Name, bool Enabled);
   void getDefaultFeatures(llvm::StringMap<bool> &Features) const override;
   bool hasFeature(StringRef Feature) const override;
   bool handleTargetFeatures(std::vector<std::string> &Features,
                             DiagnosticsEngine &Diags) override;
   StringRef getABI() const override {
-    if (getTriple().getArch() == llvm::Triple::x86_64 && SSELevel >= AVX)
+    if (getTriple().getArch() == llvm::Triple::x86_64 && SSELevel >= AVX512F)
+      return "avx512";
+    else if (getTriple().getArch() == llvm::Triple::x86_64 && SSELevel >= AVX)
       return "avx";
     else if (getTriple().getArch() == llvm::Triple::x86 &&
              MMX3DNowLevel == NoMMX3DNow)
       return "no-mmx";
     return "";
   }
   bool setCPU(const std::string &Name) override {
     CPU = llvm::StringSwitch<CPUKind>(Name)

lib/CodeGen/TargetInfo.cpp

public:
   llvm::Value *EmitVAArg(llvm::Value *VAListAddr, QualType Ty,
                          CodeGenFunction &CGF) const override;
 
   bool has64BitPointers() const {
     return Has64BitPointers;
   }
 
   bool hasAVX() const {
-    return ABI == "avx";
+    return hasAVX512() || ABI == "avx";
+  }
+  bool hasAVX512() const {
+    return ABI == "avx512";
   }
 };
 
 /// WinX86_64ABIInfo - The Windows X86_64 ABI information.
 class WinX86_64ABIInfo : public ABIInfo {
 
   ABIArgInfo classify(QualType Ty, unsigned &FreeSSERegs,
                       bool IsReturnType) const;
else
       Sig = (0xeb << 0) |  // jmp rel8
             (0x06 << 8) |  //           .+0x08
             ('F' << 16) |
             ('T' << 24);
     return llvm::ConstantInt::get(CGM.Int32Ty, Sig);
   }
 
   unsigned getOpenMPSimdDefaultAlignment(QualType) const override {
+    // FIXME: What about AVX512?
     return getABIInfo().hasAVX() ? 32 : 16;
   }
 };
 
 class PS4TargetCodeGenInfo : public X86_64TargetCodeGenInfo {
 public:
   PS4TargetCodeGenInfo(CodeGen::CodeGenTypes &CGT, StringRef ABI)
     : X86_64TargetCodeGenInfo(CGT, ABI) {}
 
if (Size == 32) {
         Current = Integer;
       else
         Current = SSE;
 
       // If this type crosses an eightbyte boundary, it should be
       // split.
       if (OffsetBase && OffsetBase != 64)
         Hi = Lo;
-    } else if (Size == 128 || (hasAVX() && isNamedArg && Size == 256)) {
+    } else if (Size == 128 || (isNamedArg && (hasAVX() && Size == 256) ||
+                               (hasAVX512() && Size == 512))) {
       // 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.
       //
       // Note that per 3.5.7 of AMD64-ABI, 256-bit args are only passed in
       // registers if they are "named", i.e. not part of the "..." of a
       // variadic function.
+      //
+      // Similarly, per 3.2.3. of the AVX512 draft, 512-bit "named" args are
+      // split into eight eightbyte chunks, one SSE and seven SSEUP.
       Lo = SSE;
       Hi = SSEUp;
     }
     return;
   }
 
   if (const ComplexType *CT = Ty->getAs<ComplexType>()) {
     QualType ET = getContext().getCanonicalType(CT->getElementType());
ABIArgInfo X86_64ABIInfo::getIndirectReturnResult(QualType Ty) const {
   }
 
   return ABIArgInfo::getIndirect(0);
 }
 
 bool X86_64ABIInfo::IsIllegalVectorType(QualType Ty) const {
   if (const VectorType *VecTy = Ty->getAs<VectorType>()) {
     uint64_t Size = getContext().getTypeSize(VecTy);
-    unsigned LargestVector = hasAVX() ? 256 : 128;
+    unsigned LargestVector = hasAVX512() ? 512 : (hasAVX() ? 256 : 128);
     if (Size <= 64 || Size > LargestVector)
       return true;
   }
 
   return false;
 }
 
 ABIArgInfo X86_64ABIInfo::getIndirectResult(QualType Ty,

test/CodeGen/x86_64-arguments.c

 // RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s | \
-// RUN:   FileCheck %s -check-prefix=CHECK -check-prefix=SSE
+// RUN:   FileCheck %s -check-prefix=CHECK -check-prefix=SSE -check-prefix=NO-AVX512
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s -target-feature +avx | \
-// RUN:   FileCheck %s -check-prefix=CHECK -check-prefix=AVX
+// RUN:   FileCheck %s -check-prefix=CHECK -check-prefix=AVX -check-prefix=NO-AVX512
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s -target-feature +avx512f | \
+// RUN:   FileCheck %s -check-prefix=CHECK -check-prefix=AVX -check-prefix=AVX512
 #include <stdarg.h>
 
 // CHECK-LABEL: define signext i8 @f0()
 char f0(void) {
   return 0;
 }
 
 // CHECK-LABEL: define signext i16 @f1()

 void test54_helper(__m256, ...);
 __m256 x54;
 void test54() {
   test54_helper(x54, x54, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0i);
   test54_helper(x54, x54, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0i);
 }
 // AVX: @test54_helper(<8 x float> {{%[a-zA-Z0-9]+}}, <8 x float> {{%[a-zA-Z0-9]+}}, double 1.000000e+00, double 1.000000e+00, double 1.000000e+00, double 1.000000e+00, double 1.000000e+00, double {{%[a-zA-Z0-9]+}}, double {{%[a-zA-Z0-9]+}})
 // AVX: @test54_helper(<8 x float> {{%[a-zA-Z0-9]+}}, <8 x float> {{%[a-zA-Z0-9]+}}, double 1.000000e+00, double 1.000000e+00, double 1.000000e+00, double 1.000000e+00, double 1.000000e+00, double 1.000000e+00, { double, double }* byval align 8 {{%[a-zA-Z0-9]+}})
+
+typedef float __m512 __attribute__ ((__vector_size__ (64)));
+typedef struct {
+  __m512 m;
+} s512;
+
+s512 x55;
+__m512 x56;
+
+// Even on AVX512, aggregates of size larger than four eightbytes have class
+// MEMORY (AVX512 draft 0.3 3.2.3p2 Rule 1).
+//
+// CHECK: declare void @f55(%struct.s512* byval align 64)
+void f55(s512 x);
+
+// However, __m512 has type SSE/SSEUP on AVX512.
+//
+// AVX512: declare void @f56(<16 x float>)
+// NO-AVX512: declare void @f56(<16 x float>* byval align 64)
+void f56(__m512 x);
+void f57() { f55(x55); f56(x56); }
+
+// Like for __m128 on AVX, check that the struct below is passed
+// in the same way regardless of AVX512 being used.
+//
+// CHECK: declare void @f58(%struct.t256* byval align 32)
+typedef struct t256 {
+  __m256 m;
+  __m256 n;
+} two256;
+
+extern void f58(two256 s);
+void f59(two256 s) {
+  f58(s);
+}
+
+// CHECK: declare void @f60(%struct.sat256* byval align 32)
+typedef struct at256 {
+  __m256 array[2];
+} Atwo256;
+typedef struct sat256 {
+  Atwo256 x;
+} SAtwo256;
+
+extern void f60(SAtwo256 s);
+void f61(SAtwo256 s) {
+  f60(s);
+}
+
+// AVX512: @f62_helper(i32 0, <16 x float> {{%[a-zA-Z0-9]+}}, double 1.000000e+00, double 1.000000e+00, double 1.000000e+00, double 1.000000e+00, double 1.000000e+00, double 1.000000e+00, double {{%[a-zA-Z0-9]+}}, double {{%[a-zA-Z0-9]+}})
+void f62_helper(int, ...);
+__m512 x62;
+void f62() {
+  f62_helper(0, x62, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0i);
+}
+
+// Like for __m256 on AVX, we always pass __m512 in memory, and don't
+// need to use the register save area.
+//
+// AVX512-LABEL: define void @f63
+// AVX512-NOT: br i1
+// AVX512: ret void
+void f63(__m512 *m, __builtin_va_list argList) {
+  *m = __builtin_va_arg(argList, __m512);
+}
+
+// AVX512: @f64_helper(<16 x float> {{%[a-zA-Z0-9]+}}, <16 x float> {{%[a-zA-Z0-9]+}}, double 1.000000e+00, double 1.000000e+00, double 1.000000e+00, double 1.000000e+00, double 1.000000e+00, double {{%[a-zA-Z0-9]+}}, double {{%[a-zA-Z0-9]+}})
+// AVX512: @f64_helper(<16 x float> {{%[a-zA-Z0-9]+}}, <16 x float> {{%[a-zA-Z0-9]+}}, double 1.000000e+00, double 1.000000e+00, double 1.000000e+00, double 1.000000e+00, double 1.000000e+00, double 1.000000e+00, { double, double }* byval align 8 {{%[a-zA-Z0-9]+}})
+void f64_helper(__m512, ...);
+__m512 x64;
+void f64() {
+  f64_helper(x64, x64, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0i);
+  f64_helper(x64, x64, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0i);
+}