[SystemZ] Improve handling of vector alignments

[jonpa]

Summary

With these first experiments, I see:

• Clang seems to now emit a padding in the case of no-vx, like

%struct.S = type { i32, [12 x i8], <4 x i32> }

instead of

%struct.S = type { i32, <4 x i32> }

This should be equivalent, in the final output, I would hope, although I have not looked into that handling in detail.

• As seen in function-attributes-01.ll, if the IR has no alignment specified, it is 8-byte aligned now even with -vector.

• vec-abi-align.ll needed updating to include the padding in the struct - as now expected from clang - to work...

It seems that SPEC builds identically both for z15 and zEC12, which is promising. I however don't quite know if this is actually correct or not: what if some optimizer want to build a vector and adds the wrong alignment in the no-vx case? Is the alignment as specified by the front end and found in the I/R enough to make this work in all cases? I am a little worried that some optimizer might look up that alignment value in DataLayout and use that instead...

[uweigand]

It seems that SPEC builds identically both for z15 and zEC12, which is promising.

This is already good news. However, I believe SPEC doesn't really use any attribute((vector_size)) so that might not tell us much. Can you do the same exercise with e.g. the llvm-test-suite? That does at least have a few instances of those types. It might also be good to run the ABI compat test suite (out of the GCC test suite).

I however don't quite know if this is actually correct or not: what if some optimizer want to build a vector and adds the wrong alignment in the no-vx case? Is the alignment as specified by the front end and found in the I/R enough to make this work in all cases? I am a little worried that some optimizer might look up that alignment value in DataLayout and use that instead...

I mean, the front-end *should* create IR that makes the alignment requirement clear (via align attributes, and explicit padding within structs), and the optimizers need to respect that anyway.

llvm/lib/Target/SystemZ/SystemZTargetMachine.cpp

46

We should remove the CPU and FS arguments here, to make it obvious that the data layout should depend *only* on the Triple.

llvm/test/CodeGen/SystemZ/vec-abi-align.ll

45

This test case now isn't quite testing any more what it was initially supposed to test :-)

Maybe to fully test the new behavior, we should test all four combinations of S_novx vs. S_vx and -vector vs. +vector? I.e. have both CHECK-VECTOR and CHECK-NOVECTOR lines in both fun1 and fun2.

[jonpa]

Patch updated.

This is already good news. However, I believe SPEC doesn't really use any attribute((vector_size)) so that might not tell us much. Can you do the same exercise with e.g. the llvm-test-suite? That does at least have a few instances of those types. It might also be good to run the ABI compat test suite (out of the GCC test suite).

I am quite sure that the llvm test-suite should pass with this patch (I rebuilt all files containing the attribute((vector_size)) and found no diff. For some reason I first got into trouble by merely adding -save-temps: undefined references at the link stage).

I tried running the GCC test suite as well, but sofar I have not been able to run it properly, I think (gcc failed the api check against itself).

[uweigand]

LGTM, thanks!

clang/lib/Basic/Targets/SystemZ.h

SystemZTargetInfo(const llvm::Triple &Triple, const TargetOptions &)
     LongDoubleAlign = 64;
     LongDoubleFormat = &llvm::APFloat::IEEEquad();
     DefaultAlignForAttributeAligned = 64;
     MinGlobalAlign = 16;
     if (Triple.isOSzOS()) {
       // All vector types are default aligned on an 8-byte boundary, even if the
       // vector facility is not available. That is different from Linux.
       MaxVectorAlign = 64;
-      // Compared to Linux/ELF, the data layout differs only in some details:
-      // - name mangling is GOFF
-      // - 128 bit vector types are 64 bit aligned
+      // Compared to Linux/ELF, the data layout differs only in that name
+      // mangling is GOFF.
       resetDataLayout(
           "E-m:l-i1:8:16-i8:8:16-i64:64-f128:64-v128:64-a:8:16-n32:64");
     } else
-      resetDataLayout("E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-a:8:16-n32:64");
+      resetDataLayout("E-m:e-i1:8:16-i8:8:16-i64:64-f128:64"
+                      "-v128:64-a:8:16-n32:64");
     MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64;
     HasStrictFP = true;
   }
 
   void getTargetDefines(const LangOptions &Opts,
                         MacroBuilder &Builder) const override;
 
   ArrayRef<Builtin::Info> getTargetBuiltins() const override;
for (const auto &Feature : Features) {
         HasTransactionalExecution = true;
       else if (Feature == "+vector")
         HasVector = true;
       else if (Feature == "+soft-float")
         SoftFloat = true;
     }
     HasVector &= !SoftFloat;
 
-    // If we use the vector ABI, vector types are 64-bit aligned.
-    if (HasVector && !getTriple().isOSzOS()) {
+    // If we use the vector ABI, vector types are 64-bit aligned. The
+    // DataLayout string is always set to this alignment as it is not a
+    // requirement that it follows the alignment emitted by the front end. It
+    // is assumed generally that the Datalayout should reflect only the
+    // target triple and not any specific feature.
+    if (HasVector && !getTriple().isOSzOS())
       MaxVectorAlign = 64;
-      resetDataLayout("E-m:e-i1:8:16-i8:8:16-i64:64-f128:64"
-                      "-v128:64-a:8:16-n32:64");
-    }
+
     return true;
   }
 
   bool hasFeature(StringRef Feature) const override;
 
   CallingConvCheckResult checkCallingConvention(CallingConv CC) const override {
     switch (CC) {
     case CC_C:

clang/test/CodeGen/SystemZ/align-systemz-02.c

+// RUN: %clang_cc1 -triple s390x-linux-gnu %s -o - -target-feature +vector -emit-llvm \
+// RUN:    | FileCheck %s -check-prefix=VECIR
+// RUN: %clang_cc1 -triple s390x-linux-gnu %s -o - -target-feature +vector -emit-obj -S \
+// RUN:    | FileCheck %s -check-prefix=VECASM
+// RUN: %clang_cc1 -triple s390x-linux-gnu %s -o - -target-feature -vector -emit-llvm \
+// RUN:    | FileCheck %s -check-prefix=SCALIR
+// RUN: %clang_cc1 -triple s390x-linux-gnu %s -o - -target-feature -vector -emit-obj -S \
+// RUN:    | FileCheck %s -check-prefix=SCALASM
+
+typedef __attribute__((vector_size(16))) signed int vec_sint;
+
+volatile vec_sint Vsi1Glob;
+
+struct S {
+  int A;
+  vec_sint Vsi;
+} GlobS;
+
+void fun() {
+  GlobS.Vsi = Vsi1Glob;
+}
+
+// VECIR: %struct.S = type { i32, <4 x i32> }
+// VECIR: @Vsi1Glob = global <4 x i32> zeroinitializer, align 8
+// VECIR: @GlobS = global %struct.S zeroinitializer, align 8
+// VECIR: %0 = load volatile <4 x i32>, ptr @Vsi1Glob, align 8
+// VECIR: store <4 x i32> %0, ptr getelementptr inbounds (%struct.S, ptr @GlobS, i32 0, i32 1), align 8
+
+// VECASM:      lgrl %r1, Vsi1Glob@GOT
+// VECASM-NEXT: vl   %v0, 0(%r1), 3
+// VECASM-NEXT: lgrl %r1, GlobS@GOT
+// VECASM-NEXT: vst  %v0, 8(%r1), 3
+//
+// VECASM:   .globl  Vsi1Glob
+// VECASM:   .p2align        3
+// VECASM: Vsi1Glob:
+// VECASM:   .space  16
+// VECASM:   .globl  GlobS
+// VECASM:   .p2align        3
+// VECASM: GlobS:
+// VECASM:   .space  24
+
+// SCALIR: %struct.S = type { i32, [12 x i8], <4 x i32> }
+// SCALIR: @Vsi1Glob = global <4 x i32> zeroinitializer, align 16
+// SCALIR: @GlobS = global %struct.S zeroinitializer, align 16
+// SCALIR: %0 = load volatile <4 x i32>, ptr @Vsi1Glob, align 16
+// SCALIR: store <4 x i32> %0, ptr getelementptr inbounds (%struct.S, ptr @GlobS, i32 0, i32 2), align 16
+
+// SCALASM:      lgrl    %r1, Vsi1Glob@GOT
+// SCALASM-NEXT: l       %r0, 0(%r1)
+// SCALASM-NEXT: l       %r2, 4(%r1)
+// SCALASM-NEXT: l       %r3, 8(%r1)
+// SCALASM-NEXT: l       %r4, 12(%r1)
+// SCALASM-NEXT: lgrl    %r1, GlobS@GOT
+// SCALASM-NEXT: st      %r4, 28(%r1)
+// SCALASM-NEXT: st      %r3, 24(%r1)
+// SCALASM-NEXT: st      %r2, 20(%r1)
+// SCALASM-NEXT: st      %r0, 16(%r1)
+//
+// SCALASM:   .globl  Vsi1Glob
+// SCALASM:   .p2align        4
+// SCALASM: Vsi1Glob:
+// SCALASM:   .space  16
+// SCALASM:   .globl  GlobS
+// SCALASM:   .p2align        4
+// SCALASM: GlobS:
+// SCALASM:   .space  32
+

clang/test/CodeGen/target-data.c

 // RUN: %clang_cc1 -triple hexagon-unknown -o - -emit-llvm %s | \
 // RUN: FileCheck %s -check-prefix=HEXAGON
 // HEXAGON: target datalayout = "e-m:e-p:32:32:32-a:0-n16:32-i64:64:64-i32:32:32-i16:16:16-i1:8:8-f32:32:32-f64:64:64-v32:32:32-v64:64:64-v512:512:512-v1024:1024:1024-v2048:2048:2048"
 
 // RUN: %clang_cc1 -triple s390x-unknown -target-cpu z10 -o - -emit-llvm %s | \
 // RUN: FileCheck %s -check-prefix=SYSTEMZ
 // RUN: %clang_cc1 -triple s390x-unknown -target-cpu z13 -target-feature +soft-float -o - -emit-llvm %s | \
 // RUN: FileCheck %s -check-prefix=SYSTEMZ
-// SYSTEMZ: target datalayout = "E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-a:8:16-n32:64"
+// SYSTEMZ: target datalayout = "E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-v128:64-a:8:16-n32:64"
 
 // RUN: %clang_cc1 -triple s390x-unknown -target-cpu z13 -o - -emit-llvm %s | \
 // RUN: FileCheck %s -check-prefix=SYSTEMZ-VECTOR
 // SYSTEMZ-VECTOR: target datalayout = "E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-v128:64-a:8:16-n32:64"
 
 // RUN: %clang_cc1 -triple s390x-none-zos -target-cpu z10 -o - -emit-llvm %s | \
 // RUN: FileCheck %s -check-prefix=ZOS
 // RUN: %clang_cc1 -triple s390x-none-zos -target-cpu z13 -target-feature +soft-float -o - -emit-llvm %s | \

llvm/lib/Target/SystemZ/SystemZTargetMachine.cpp

extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeSystemZTarget() {
   initializeSystemZShortenInstPass(PR);
   initializeSystemZLongBranchPass(PR);
   initializeSystemZLDCleanupPass(PR);
   initializeSystemZShortenInstPass(PR);
   initializeSystemZPostRewritePass(PR);
   initializeSystemZTDCPassPass(PR);
 }
 
-// Determine whether we use the vector ABI.
-static bool UsesVectorABI(StringRef CPU, StringRef FS) {
-  // We use the vector ABI whenever the vector facility is avaiable.
-  // This is the case by default if CPU is z13 or later, and can be
-  // overridden via "[+-]vector" feature string elements.
-  bool VectorABI = true;
-  bool SoftFloat = false;
-  if (CPU.empty() || CPU == "generic" ||
-      CPU == "z10" || CPU == "z196" || CPU == "zEC12" ||
-      CPU == "arch8" || CPU == "arch9" || CPU == "arch10")
-    VectorABI = false;
-
-  SmallVector<StringRef, 3> Features;
-  FS.split(Features, ',', -1, false /* KeepEmpty */);
-  for (auto &Feature : Features) {
-    if (Feature == "vector" || Feature == "+vector")
-      VectorABI = true;
-    if (Feature == "-vector")
-      VectorABI = false;
-    if (Feature == "soft-float" || Feature == "+soft-float")
-      SoftFloat = true;
-    if (Feature == "-soft-float")
-      SoftFloat = false;
-  }
-
-  return VectorABI && !SoftFloat;
-}
-
 static std::string computeDataLayout(const Triple &TT, StringRef CPU,
                                      StringRef FS) {
-  bool VectorABI = UsesVectorABI(CPU, FS);
   std::string Ret;
 
   // Big endian.
   Ret += "E";
 
   // Data mangling.
   Ret += DataLayout::getManglingComponent(TT);
 
   // Make sure that global data has at least 16 bits of alignment by
   // default, so that we can refer to it using LARL.  We don't have any
   // special requirements for stack variables though.
   Ret += "-i1:8:16-i8:8:16";
 
   // 64-bit integers are naturally aligned.
   Ret += "-i64:64";
 
   // 128-bit floats are aligned only to 64 bits.
   Ret += "-f128:64";
 
-  // When using the vector ABI on Linux, 128-bit vectors are also aligned to 64
-  // bits. On z/OS, vector types are always aligned to 64 bits.
-  if (VectorABI || TT.isOSzOS())
-    Ret += "-v128:64";
+  // The DataLayout string always holds a vector alignment of 64 bits, see
+  // comment in clang/lib/Basic/Targets/SystemZ.h.
 
   // We prefer 16 bits of aligned for all globals; see above.
   Ret += "-a:8:16";
 
   // Integer registers are 32 or 64 bits.
   Ret += "-n32:64";
 
   return Ret;

llvm/test/CodeGen/SystemZ/function-attributes-01.ll

 entry:
   %0 = load double, double* %A
   ret double %0
 }
 
 attributes #3 = { "target-cpu"="z14" "target-features"="+vector" "use-soft-float"="false" }
 define <2 x double> @fun3(<2 x double>* %A) #3 {
 ; CHECK-LABEL:     fun3:
-; DEFAULT:         vl %v24, 0(%r2), 4
+; DEFAULT:         vl %v24, 0(%r2), 3
 ; SOFT-FLOAT:      lg %r0, 0(%r2)
 ; SOFT-FLOAT-NEXT: lg %r3, 8(%r2)
 ; SOFT-FLOAT-NEXT: lgr %r2, %r0
-; NO-SOFT-FL:      vl %v24, 0(%r2), 4
+; NO-SOFT-FL:      vl %v24, 0(%r2), 3
 ; NO-VECTOR:       ld %f0, 0(%r2)
 ; NO-VECTOR-NEXT:  ld %f2, 8(%r2)
 ; CHECK-NEXT:      br %r14
 entry:
   %0 = load <2 x double>, <2 x double>* %A
   ret <2 x double> %0
 }
 

 entry:
   %0 = load <2 x double>, <2 x double>* %A
   ret <2 x double> %0
 }
 
 attributes #7 = { "target-cpu"="zEC12" "target-features"="+vector" "use-soft-float"="false" }
 define <2 x double> @fun7(<2 x double>* %A) #7 {
 ; CHECK-LABEL:     fun7:
-; DEFAULT:         vl %v24, 0(%r2), 4
+; DEFAULT:         vl %v24, 0(%r2), 3
 ; SOFT-FLOAT:      lg %r0, 0(%r2)
 ; SOFT-FLOAT-NEXT: lg %r3, 8(%r2)
 ; SOFT-FLOAT-NEXT: lgr %r2, %r0
-; NO-SOFT-FL:      vl %v24, 0(%r2), 4
+; NO-SOFT-FL:      vl %v24, 0(%r2), 3
 ; NO-VECTOR:       ld %f0, 0(%r2)
 ; NO-VECTOR-NEXT:  ld %f2, 8(%r2)
 ; CHECK-NEXT:      br %r14
 entry:
   %0 = load <2 x double>, <2 x double>* %A
   ret <2 x double> %0
 }

llvm/test/CodeGen/SystemZ/vec-abi-align.ll

 ; RUN:   FileCheck -check-prefix=CHECK-NOVECTOR %s
 ; RUN: llc < %s -mtriple=s390x-linux-gnu -mcpu=z13 \
 ; RUN:   -mattr=soft-float,-soft-float | \
 ; RUN:   FileCheck -check-prefix=CHECK-VECTOR %s
 ; RUN: llc < %s -mtriple=s390x-linux-gnu -mcpu=z13 \
 ; RUN:   -mattr=-soft-float,soft-float | \
 ; RUN:   FileCheck -check-prefix=CHECK-NOVECTOR %s
 
-%struct.S = type { i8, <2 x i64> }
+%struct.S_novx = type { i8, [15 x i8], <2 x i64> }
+%struct.S_vx = type { i8, <2 x i64> }
 
-define void @test(%struct.S* %s) nounwind {
-; CHECK-VECTOR-LABEL: @test
+define void @fun1(%struct.S_vx* %s) nounwind {
+; CHECK-VECTOR-LABEL: @fun1
 ; CHECK-VECTOR: vl %v0, 8(%r2)
-; CHECK-NOVECTOR-LABEL: @test
+  %ptr = getelementptr %struct.S_vx, %struct.S_vx* %s, i64 0, i32 1
+  %vec = load <2 x i64>, <2 x i64>* %ptr
+  %add = add <2 x i64> %vec, <i64 1, i64 1>
+  store <2 x i64> %add, <2 x i64>* %ptr
+  ret void
+}
+
+define void @fun2(%struct.S_novx* %s) nounwind {
+; CHECK-NOVECTOR-LABEL: @fun2
 ; CHECK-NOVECTOR-DAG: agsi 16(%r2), 1
 ; CHECK-NOVECTOR-DAG: agsi 24(%r2), 1
-  %ptr = getelementptr %struct.S, %struct.S* %s, i64 0, i32 1
+  %ptr = getelementptr %struct.S_novx, %struct.S_novx* %s, i64 0, i32 2
   %vec = load <2 x i64>, <2 x i64>* %ptr
   %add = add <2 x i64> %vec, <i64 1, i64 1>
   store <2 x i64> %add, <2 x i64>* %ptr
   ret void
 }