[LLVMdev] Shuffle combine

[Mon Ping Wang]

Hi,

Feel free to file a bug report if you would like.  I meant to clean  
this up after I made the change to extended the shuffle but never got  
around to it.  I'll try to deal with it sometime next week if time  
permits.

   -- Mon Ping

  --

On Apr 3, 2009, at 11:57 AM, Stefanus Du Toit wrote:

> Hi Nicolas,
>
> On 2-Apr-09, at 6:04 PM, Nicolas Capens wrote:
>> Thanks for verifying this. Could you patch this or should I open a  
>> new bug report and find a generic solution first?
>
> I don't have write access so the best I could do would be to submit  
> a patch, and I'm crazy busy at the moment.
>
> I actually think the check I described below is fine and would fix  
> this bug (but haven't tested it), but the function could be improved  
> to operate on more cases. I'd actually be happy to work on a patch  
> for this in my spare time since I haven't hacked on LLVM in a while,  
> but if you're interested in doing it let me know. In any case, the  
> one-line patch (plus a test case, and probably an assert to do the  
> bounds check in debug mode) would probably be worthwhile to do right  
> away.
>
> Stefanus
>
>>
>> Cheers,
>>
>> Nicolas
>>
>> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev- 
>> bounces at cs.uiuc.edu] On Behalf Of Stefanus Du Toit
>> Sent: woensdag 1 april 2009 18:59
>> To: LLVM Developers Mailing List
>> Subject: Re: [LLVMdev] Shuffle combine
>>
>> On 1-Apr-09, at 12:42 PM, Nicolas Capens wrote:
>> Hi Stefanus,
>>
>> Thanks for the info. I still think it’s a bug though. Take for  
>> example a case where the vectors each have four elements. The  
>> values in Mask[] can range from 0 to 7, while HLSMask only has 4  
>> elements. So LHSMask[Mask[i]] can go out of bounds, no?
>>
>> Good point! One easy way to fix this would be to use:
>>
>> if (Mask[i] >= e) {
>>
>> instead. The code could also be improved to consider the new mask  
>> equal to lhsmask/mask in places where they both refer to undefined  
>> elements.
>>
>> Stefanus
>>
>> Cheers,
>>
>> Nicolas
>>
>> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev- 
>> bounces at cs.uiuc.edu] On Behalf Of Stefanus Du Toit
>> Sent: woensdag 1 april 2009 16:31
>> To: LLVM Developers Mailing List
>> Subject: Re: [LLVMdev] Shuffle combine
>>
>> Hi Nicolas,
>>
>> On 1-Apr-09, at 7:34 AM, Nicolas Capens wrote:
>> I’m having some trouble understanding the following lines in  
>> InstructionCombining.cpp, which possibly contain a bug:
>>
>> if (Mask[i] >= 2*e)
>>     NewMask.push_back(2*e);
>> else
>>     NewMask.push_back(LHSMask[Mask[i]]);
>>
>> When Mask[i] is bigger than the size of LHSMask it reads out of  
>> bounds on that last line. I believe the first line is there to try  
>> to prevent that but then it should be comparing to LHSMask.size()  
>> not 2*e (e being Mask.size()).
>>
>> Upon quick inspection of the code, I don't think this is  
>> necessarily a bug right now, but the function could be improved.
>>
>> I believe this stems (in part at least) from the fact that  
>> shufflevector used to require that the sources and the mask have  
>> the same size, which was changed a few months ago to allow an  
>> arbitrarily-sized mask.
>>
>> This would be a bug all throughout this function (which generally  
>> assumes this is still the case), if the function didn't do the  
>> following check early:
>>
>>   unsigned VWidth = cast<VectorType>(SVI.getType())->getNumElements 
>> ();
>>
>>   if (VWidth != cast<VectorType>(LHS->getType())->getNumElements())
>>     return 0;
>>
>> In other words, if the mask size is not equal to the number of  
>> elements in the vectors, it skips this transformation.
>>
>> Because the LHS is a shufflevector in the part of the code you are  
>> mentioning, and the result of a shufflevector has the same number  
>> of elements as its mask, you are actually guaranteed that  
>> LHSMask.size() == Mask.size(), because LHSMask.size() == LHS- 
>> >getNumElements() == Mask.size().
>>
>> It shouldn't be too hard to relax the constraint that this  
>> optimization requires the number of elements being shuffle to be  
>> equal to the mask size, but it'll probably take some careful testing!
>>
>> Stefanus
>>
>> --
>> Stefanus Du Toit <stefanus.dutoit at rapidmind.com>
>>   RapidMind Inc.
>>   phone: +1 519 885 5455 x116 -- fax: +1 519 885 1463
>>
>>
>>
>> <ATT00001.txt>
>>
>> --
>> Stefanus Du Toit <stefanus.dutoit at rapidmind.com>
>>   RapidMind Inc.
>>   phone: +1 519 885 5455 x116 -- fax: +1 519 885 1463
>>
>>
>>
>> <ATT00001.txt>
>
> --
> Stefanus Du Toit <stefanus.dutoit at rapidmind.com>
>   RapidMind Inc.
>   phone: +1 519 885 5455 x116 -- fax: +1 519 885 1463
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090404/6cc0394b/attachment.html>