[PATCH] D131158: [SystemZ] Improve handling of vector alignments

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

> 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.



================
Comment at: llvm/lib/Target/SystemZ/SystemZTargetMachine.cpp:46
 static std::string computeDataLayout(const Triple &TT, StringRef CPU,
                                      StringRef FS) {
   std::string Ret;
----------------
We should remove the `CPU` and `FS` arguments here, to make it obvious that the data layout should depend *only* on the Triple.


================
Comment at: llvm/test/CodeGen/SystemZ/vec-abi-align.ll:45
+%struct.S_novx = type { i8, [15 x i8], <2 x i64> }
+%struct.S_vx = type { i8, <2 x i64> }
 
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131158/new/

https://reviews.llvm.org/D131158