[cfe-dev] __builtin_shufflevector vs. llvm shufflevector

[Eli Friedman]

On Wed, Feb 8, 2012 at 5:35 PM, Florian Pflug <fgp at phlo.org> wrote:
> On Feb8, 2012, at 21:08 , Eli Friedman wrote:
>> On Wed, Feb 8, 2012 at 7:35 AM, Florian Pflug <fgp at phlo.org> wrote:
>>> Does that mean that the initial plan was abolished and masks with
>>> undef element aren't supported in clang, or is this simply a signed
>>> vs. unsigned bug in Sema::SemaBuiltinShuffleVector (as the phrasing
>>> of the error message makes me suspect)?
>>
>> I'm pretty sure this is just an accident... I wasn't really thinking
>> about undef shuffle indexes when I wrote the code in SemaChecking.cpp.
>
> I tried relaxing the check during sema checking to allow -1, and fixed
> codegen to consistently translate index value -1 to undef. And voila,
> it works. __builtin_shufflevector(v, v, 1, -1) is now translated to
>
>  shufflevector <2 x i8> %v, <2 x i8> undef, <2 x i32> <i32 1, i32 undef>
>
> Patch is attached.
>
> The patch also changes the error message for out-of-range index values
> to be more specific about the allowed range (-1 to 2*N), changes the
> diag code from err_shufflevector_argument_too_large to
> err_shufflevector_argument_out_of_range since the former seemed strange
> for signed values, and finally verifies that the indices fit into 32-bit
> signed integers. The previous check for getActiveBits() <= 64 seemed
> overly lax, since the indices are later treated as 32-bit quantities,
> by both clang and llvm.

Index: lib/CodeGen/CGExprScalar.cpp
===================================================================
--- lib/CodeGen/CGExprScalar.cpp	(revision 149985)
+++ lib/CodeGen/CGExprScalar.cpp	(working copy)
@@ -785,10 +785,13 @@
   llvm::VectorType *VTy = cast<llvm::VectorType>(V1->getType());
   SmallVector<llvm::Constant*, 32> indices;
   for (unsigned i = 2; i < E->getNumSubExprs(); i++) {
-    unsigned Idx = E->getShuffleMaskIdx(CGF.getContext(), i-2);
+    int Idx = E->getShuffleMaskIdx(CGF.getContext(), i-2);

Is this patch missing some changes?  This change doesn't match the
signature of getShuffleMaskIdx, and getShuffleMaskIdx has an assertion
which should break this... (although it might mostly work with
assertions disabled).

Index: lib/Sema/SemaChecking.cpp
===================================================================
--- lib/Sema/SemaChecking.cpp	(revision 149985)
+++ lib/Sema/SemaChecking.cpp	(working copy)
@@ -1247,9 +1247,10 @@
                   diag::err_shufflevector_nonconstant_argument)
                 << TheCall->getArg(i)->getSourceRange());

-    if (Result.getActiveBits() > 64 || Result.getZExtValue() >= numElements*2)
+    if ((Result.getMinSignedBits() > 32) ||
+        (Result.getSExtValue() < -1) || (Result.getSExtValue() >=
numElements*2))
       return ExprError(Diag(TheCall->getLocStart(),
-                  diag::err_shufflevector_argument_too_large)
+                  diag::err_shufflevector_argument_out_of_range)
                << TheCall->getArg(i)->getSourceRange());
   }

Ideally, this check would correctly handle both the case where Result
is signed and the case where it is unsigned... you're changing it from
assuming the value is unsigned to assuming the value is signed.

-Eli