Hi Stefanus, Thanks for verifying this. Could you patch this or should I open a new bug report and find a generic solution first? 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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090403/27039d56/attachment.html>
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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090403/ae339110/attachment.html>
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>