[LLVMdev] Shuffle combine

[Stefanus Du Toit]

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



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