[PATCH] D106120: [PowerPC] Implement vector bool/pixel initialization under -faltivec-src-compat=xl

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai requested changes to this revision.
nemanjai added a comment.
This revision now requires changes to proceed.

Why are `vector-scalar-altivec-init.c` and `vector-scalar-altivec-init2.c` added? There is no initialization of `vector bool` or `vector pixel` in them so I don't really see the need to add them. If it is just to test that the existing behaviour doesn't change for those, you can simply add two run lines for `xl` and `mixed` to existing test cases.

Also, please unify `vector-bool-pixel-altivec-init.c` and `vector-bool-pixel-altivec-init2.c` into a single test case. It is not immediately obvious to the reader that the difference is parenthesized vs. unparenthesized initialization.



================
Comment at: clang/include/clang/Sema/Sema.h:6097
+  // option, these types also splat the scalar value.
+  bool ShouldSplatAltivecScalarInCast(Sema &Self, const VectorType *VecTy);
+
----------------
Why take a `Sema &` parameter? It is called `Self` so presumably it is expected to point to `*this`. Is there a use case where it points to a different instance of `Sema`?


================
Comment at: clang/lib/Sema/SemaCast.cpp:2627
 
+// Checks if we have a valid AltiVec vector type, and splats the value into
+// the vector accordingly. If a 'vector bool' or 'vector pixel' type is used
----------------
No need to repeat the comment on the implementation.


================
Comment at: clang/test/CodeGen/vector-bool-pixel-altivec-init.c:57
+  // MIXED: insertelement <8 x i16>
+  // XL: %splat.splatinsert1 = insertelement <8 x i16>
+  // XL-NEXT: %splat.splat2 = shufflevector <8 x i16> %splat.splatinsert1
----------------
It is generally not safe to hard-code names of virtual registers (`llvm::Value*`'s) in test cases. The naming is different in Release vs. Debug builds and it could also change for any other reason as no guarantee is ever made about the names.

You should test for what's important:
- The `insertelement` of the right type (save the name with `[[INS:%.*]]`)
- The use of that value for a `shufflevector` where you check the shuffle mask (presumably `zeroinitializer`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106120