[cfe-dev] __builtin_shufflevector vs. llvm shufflevector

Florian Pflug fgp at phlo.org
Thu Feb 9 04:00:39 PST 2012


On Feb9, 2012, at 06:26 , Eli Friedman wrote:
> On Wed, Feb 8, 2012 at 5:35 PM, Florian Pflug <fgp at phlo.org> wrote:
> 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).

Ups, I missed that fact that getShuffleMaskIdx() needs to be changed too.
Funnily enough, it works despite this signature mismatch, even in my
Debug+Assert built. My guess is that the mask indices usually have a width
of at least 32 bits, and that makes getZExtValue() followed by a cast from
unsigned to int equivalent to getSExtValue().

I didn't find the assertion you expect to break, though - getShuffleMaskIdx
only seems to check the index into the Mask (i.e. the second parameter N),
not the mask's contents.

Anyway, I now changed the return value to int, and made it use getSExtValue()
instead of getZExtValue(). That should be safe, since Sema has already
verified that the index fits into a signed int, right?

> 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.

Hm, I don't see why this would break for unsigned integers. Isn't the
check for (getMinSignedBits > 32) sufficient to guarantee that getSExtValue()
won't mangle the value, regardless of Result's signed-ness?

New patch attached.

best regards,
Florian Pflug
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang.shufflevector.undef.v2.patch
Type: application/octet-stream
Size: 3220 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120209/e28b5426/attachment.obj>


More information about the cfe-dev mailing list