__builtin_shufflevector question

What are the supported forms of __builtin_shufflevector supposed to be?

Comments in SemaChecking.cpp indicates these

// 1) unary, vector mask: (lhs, mask)
// 2) binary, vector mask: (lhs, rhs, mask)
// 3) binary, scalar mask: (lhs, rhs, index, …, index)

But the actual code in SemaChecking.cpp only implements 1 and 3. I tried to use 2 and it fails with “index for __builtin_shufflevector must be a constant integer” on the 3rd argument.

Additionally, official documentation indicates only form 3 is supported. http://clang.llvm.org/docs/LanguageExtensions.html

And another question, should -1 be a valid index to specify undef?

And another question, should -1 be a valid index to specify undef?

I can't think of a better way to specify undef in the general case.

I’ve started looking at cleaning this up a bit more.

I’m leaning towards removing form 2 support from CGExprScalar.cpp since it doesn’t work in Sema and therefore could never have been used/tested. I’m not even 100% sure what the semantics are supposed to be as far as widths of the individual arguments relative to each other. Thoughts?

Additionally, he error messages in SemaChecking here are a bit misleading given the overloaded nature of this instruction. For instance, we have a message that says

“first two arguments to __builtin_shufflevector must have the same type”

This error gets used for form 3 and when form 1 has a mask of non-integer integer type or a mismatch in the number of elements. Clearly form 1 needs clearer error messages for those cases. But then you end up needing to specify which form of __builtin_shufflevector in all of the error messages to avoid statements like “first two arguments must be…” which is only true for one of the forms. Or should we just have separate __builtin names for the constant index form and the mask form?

I've started looking at cleaning this up a bit more.

I'm leaning towards removing form 2 support from CGExprScalar.cpp since it
doesn't work in Sema and therefore could never have been used/tested. I'm
not even 100% sure what the semantics are supposed to be as far as widths of
the individual arguments relative to each other. Thoughts?

That's fine.

Additionally, he error messages in SemaChecking here are a bit misleading
given the overloaded nature of this instruction. For instance, we have a
message that says

"first two arguments to __builtin_shufflevector must have the same type"

This error gets used for form 3 and when form 1 has a mask of non-integer
integer type or a mismatch in the number of elements. Clearly form 1 needs
clearer error messages for those cases. But then you end up needing to
specify which form of __builtin_shufflevector in all of the error messages
to avoid statements like "first two arguments must be..." which is only true
for one of the forms. Or should we just have separate __builtin names for
the constant index form and the mask form?

Separate builtin names sounds like a good idea to me.

-Eli