[PATCH] D33053: [PowerPC] Implement vec_xxpermdi builtin.

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

Other than the few minor comments, this LGTM.



================
Comment at: lib/CodeGen/CGBuiltin.cpp:8458
+    if (getTarget().isLittleEndian()) {
+      ElemIdx0 = (~Index & 1) + 2;
+      ElemIdx1 = (~Index & 2) >> 1;
----------------
Minor nit: please add a comment explaining the expressions. For example:
```
// Element zero comes from the first input vector and element one comes from the
// second. The element indices within each vector are numbered in big-endian
// order so the shuffle mask must be adjusted for this on little endian
// platforms (i.e. index is complemented and source vector reversed).
```


================
Comment at: lib/Sema/SemaChecking.cpp:3944
+    QualType Arg21ElemTy = Arg2Ty->getAs<VectorType>()->getElementType();
+    if (Arg1ElemTy != Arg21ElemTy)
+      return Diag(BuiltinLoc, diag::err_vec_builtin_incompatible_vector)
----------------
It seems strange that we're comparing argument types above and element types of argument vectors here. Can we not just use `hasSameUnqualifiedType` like the Sema checks on `__builtin_shufflevector` do?


================
Comment at: test/CodeGen/builtins-ppc-error.c:27
+  vec_xxpermdi(vsi, vsi, index); //expected-error {{argument 3 to '__builtin_vsx_xxpermdi' must be a 2-bit unsigned literal (i.e. 0,1,2 or 3)}}
+  vec_xxpermdi(vsi, vuc, 2); //expected-error {{first two arguments to '__builtin_vsx_xxpermdi' must have the same type}}
+}
----------------
Add a test for non-vector arguments. Perhaps something like:
`vec_xxpermdi(1, 2, 3);`


https://reviews.llvm.org/D33053