Hi all,
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()).
And when Mask[i] is bigger than the mask size the shuffle combine won't work
anyway so I would replace these lines with:
if (Mask[i] >= LHSMask.size())
break;
else
NewMask.push_back(LHSMask[Mask[i]]);
But I'm not sure if I fully understand the implications. Could someone check
this for me and possibly patch it?
Thanks,
Nicolas
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20090401/060c6c6c/attachment.html>
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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090401/a892b529/attachment.html>
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?
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20090401/cbbbc72c/attachment.html>