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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.
This revision is now accepted and ready to land.

Much like Eric's comments, mine shouldn't hold up approval. Feel free to address them on the commit.

LGTM.



================
Comment at: lib/Sema/SemaChecking.cpp:3900
+// Which takes the same type of vectors (any legal vector type) for the first
+// two arguments and takes compile time constant (0-3) for the third argument.
+// If a constant larger than 3 is used, only the last two bits of it are used.
----------------
This statement doesn't belong in this comment. The Sema check is just that the third argument is a compile-time constant. There's no need to mention `0-3` or `last two bits` here. Just state what you're checking, not the details of what you're currently using it for.


================
Comment at: lib/Sema/SemaChecking.cpp:3905
+// vector short vec_xxsldwi(vector short, vector short, int);
+bool Sema::SemaBuiltinVSX(CallExpr *TheCall, unsigned NumArgs) {
+  if (TheCall->getNumArgs() < NumArgs)
----------------
All the statements in the comments as well as the code itself only handle functions that have exactly 3 parameters, the first two of which are vectors and the last an integral constant expression. I really don't see the need for the `NumArgs` parameter here. It would indeed be weird if someone down the line called it with something like:
`SemaBuiltinVSX(TheCall, 1);`
or
`SemaBuiltinVSX(TheCall, 5);`


https://reviews.llvm.org/D33053