[PATCH] D26546: [PPC] Add vec_insert4b/vec_extract4b to altivec.h

[Nemanja Ivanovic via cfe-commits]

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

Overall, I'd like to see another revision of this. This is not because I feel the patch requires major rework, but because there is a number of minor comments that I'd like to make sure are all addressed.
Just a few notes:

- Please be careful with the naming convention (mentioned in inline comments)
- Be descriptive with your asserts - imagine being a developer that gets an assert message that just says `ArgCI` - it would probably not mean much to you.
- I am pretty sure this does not require any work in Sema since AFAICT the builtins are defined to require that the third parameter is a constant (i.e. `Ii` rather than just `i`). But this should be tested (mentioned in inline comments).
- I would like to see testing cover all of the code you generate (i.e. you generate bitcasts to `<2 x i64>` and I don't see it in the tests)
- I would like to see early exits if the subtarget does not have P9Vector. This is kind of subtle, but for most builtins that lower to an intrinsic that requires some subtarget feature that isn't present, the user gets a `not able to compile this builtin yet`. As you have it implemented, it would not be the case here - it would lower to an intrinsic and we'd get an ISEL error. I think the latter situation is inferior so we should find a way to get the former. I am hoping this will not require Sema (I'm not sure if returning a `nullptr` will achieve this goal but hopefully something along those lines will). Basically, the point is that you shouldn't assume that the only way you'd get a builtin call is through the interface defined in the ABI, but that someone may actually insert a `__builtin_<whatever>` in their code.



================
Comment at: lib/CodeGen/CGBuiltin.cpp:39
+static
+int64_t clamp(int64_t value, int64_t low, int64_t high) {
+  return std::min(high, std::max(low, value));
----------------
This nit applies to all naming in this changeset: variables start with an uppercase letter and functions with a lowercase letter.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:8194
+      ShuffleElts[1] = ConstantInt::get(Int32Ty, 0);
+      Constant* ShuffleMask = llvm::ConstantVector::get(ShuffleElts);
+      // Reverse the double words in the second argument.
----------------
Minor nit: please settle on where the star goes when declaring pointers.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:8198
+
+      // reverse the index
+      index = 12 - index;
----------------
Just a general nit here - we prefer comments to be complete sentences with capitalization and punctuation.


================
Comment at: test/CodeGen/builtins-ppc-p9vector.c:1188
+// CHECK-LE-NEXT: [[T2:%.+]] = call <4 x i32> @llvm.ppc.vsx.xxinsertw(<4 x i32> {{.+}}, <2 x i64> [[T1]], i32 5)
+// CHECK-LE-NEXT: bitcast <4 x i32> T2 to <16 x i8>
+  return vec_insert4b(vuia, vuca, 7);
----------------
How does this pass? The "T2" is not enclosed in double square brackets.


Repository:
  rL LLVM

https://reviews.llvm.org/D26546