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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
