[cfe-dev] __builtin_shufflevector vs. llvm shufflevector

[Eli Friedman]

On Thu, Feb 9, 2012 at 4:00 AM, Florian Pflug <fgp at phlo.org> wrote:
> 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?

Suppose that the value in question is "-1U"... getSExtValue will treat
it as a signed -1.

-Eli