llvm dev - Jul 2013 - [LLVMdev] [llvm] r187267 - SLP Vectorier: Don't vectorize really short chains because they are already handled by the SelectionDAG store-vectorizer, which does a better job in deciding when to vectorize.

Hey Nadav,
I'd humbly suggest that rather than use 3 directly, you should add a shared
constant between these two passes, so when one changes, the other doesn't
need to be updated. It would also ensure this bit of info about what needs
to be updated isn't only contained in the comments..

On Fri, Jul 26, 2013 at 4:07 PM, Nadav Rotem <nrotem at apple.com> wrote:
> Author: nadav
> Date: Fri Jul 26 18:07:55 2013
> New Revision: 187267
>
> URL: http://llvm.org/viewvc/llvm-project?rev=187267&view=rev
> Log:
> SLP Vectorier:  Don't vectorize really short chains because they are
> already handled by the SelectionDAG store-vectorizer, which does a better
> job in deciding when to vectorize.

>
> Modified: llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp
> URL:
>
http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp?rev=187267&r1=187266&r2=187267&view=diff
>
>
=============================================================================>
--- llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp (original)
> +++ llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp Fri Jul 26
> 18:07:55 2013
> @@ -898,8 +898,12 @@ int BoUpSLP::getTreeCost() {
>    DEBUG(dbgs() << "SLP: Calculating cost for tree of size
" <<
>          VectorizableTree.size() << ".\n");
>
> -  if (!VectorizableTree.size()) {
> -    assert(!ExternalUses.size() && "We should not have any
external
> users");
> +  // Don't vectorize tiny trees. Small load/store chains or
consecutive
> stores
> +  // of constants will be vectoried in SelectionDAG in
> MergeConsecutiveStores.
> +  if (VectorizableTree.size() < 3) {
> +    if (!VectorizableTree.size()) {
> +      assert(!ExternalUses.size() && "We should not have any
external
> users");
> +    }
>      return 0;
>    }
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130726/04d9865c/attachment.html>

Hi Daniel,

Maybe my commit message was not clear. The idea is that the SelectionDAG store
vectorizer can only handle pairs. So, the number three means "more than a
pair".

Thanks,
Nadav

Sent from my iPhone
> On Jul 26, 2013, at 17:48, Daniel Berlin <dberlin at dberlin.org>
wrote:
> 
> Hey Nadav,
> I'd humbly suggest that rather than use 3 directly, you should add a
shared constant between these two passes, so when one changes, the other
doesn't need to be updated. It would also ensure this bit of info about what
needs to be updated isn't only contained in the comments..
> 
>> On Fri, Jul 26, 2013 at 4:07 PM, Nadav Rotem <nrotem at
apple.com> wrote:
>> Author: nadav
>> Date: Fri Jul 26 18:07:55 2013
>> New Revision: 187267
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=187267&view=rev
>> Log:
>> SLP Vectorier:  Don't vectorize really short chains because they
are already handled by the SelectionDAG store-vectorizer, which does a better
job in deciding when to vectorize.
>  
>> 
>> Modified: llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp
>> URL:
http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp?rev=187267&r1=187266&r2=187267&view=diff
>>
=============================================================================>>
--- llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp Fri Jul 26
18:07:55 2013
>> @@ -898,8 +898,12 @@ int BoUpSLP::getTreeCost() {
>>    DEBUG(dbgs() << "SLP: Calculating cost for tree of size
" <<
>>          VectorizableTree.size() << ".\n");
>> 
>> -  if (!VectorizableTree.size()) {
>> -    assert(!ExternalUses.size() && "We should not have
any external users");
>> +  // Don't vectorize tiny trees. Small load/store chains or
consecutive stores
>> +  // of constants will be vectoried in SelectionDAG in
MergeConsecutiveStores.
>> +  if (VectorizableTree.size() < 3) {
>> +    if (!VectorizableTree.size()) {
>> +      assert(!ExternalUses.size() && "We should not have
any external users");
>> +    }
>>      return 0;
>>    }
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130726/f5a50370/attachment.html>

Hi Nadav,

Okay.

1. The comment doesn't make this clear. I would suggest, at a minimum,
updating it to mention pairs specifically, to avoid the issue in #2

2. If the day comes when the selectiondag store vectorizer handles more
than pairs, and does so better, is anyone really going to remember this
random 3 exists in the other vectorizer?

I would posit, based on experience, the answer is "no" :)



On Fri, Jul 26, 2013 at 7:56 PM, Nadav Rotem <nrotem at apple.com> wrote:
> Hi Daniel,
>
> Maybe my commit message was not clear. The idea is that the SelectionDAG
> store vectorizer can only handle pairs. So, the number three means
"more
> than a pair".
>
> Thanks,
> Nadav
>
> Sent from my iPhone
>
> On Jul 26, 2013, at 17:48, Daniel Berlin <dberlin at dberlin.org>
wrote:
>
> Hey Nadav,
> I'd humbly suggest that rather than use 3 directly, you should add a
> shared constant between these two passes, so when one changes, the other
> doesn't need to be updated. It would also ensure this bit of info about
> what needs to be updated isn't only contained in the comments..
>
> On Fri, Jul 26, 2013 at 4:07 PM, Nadav Rotem <nrotem at apple.com>
wrote:
>
>> Author: nadav
>> Date: Fri Jul 26 18:07:55 2013
>> New Revision: 187267
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=187267&view=rev
>> Log:
>> SLP Vectorier:  Don't vectorize really short chains because they
are
>> already handled by the SelectionDAG store-vectorizer, which does a
>> better job in deciding when to vectorize.
>
>
>
>>
>> Modified: llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp
>> URL:
>>
http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp?rev=187267&r1=187266&r2=187267&view=diff
>>
>>
=============================================================================>>
--- llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Vectorize/SLPVectorizer.cpp Fri Jul 26
>> 18:07:55 2013
>> @@ -898,8 +898,12 @@ int BoUpSLP::getTreeCost() {
>>    DEBUG(dbgs() << "SLP: Calculating cost for tree of size
" <<
>>          VectorizableTree.size() << ".\n");
>>
>> -  if (!VectorizableTree.size()) {
>> -    assert(!ExternalUses.size() && "We should not have
any external
>> users");
>> +  // Don't vectorize tiny trees. Small load/store chains or
consecutive
>> stores
>> +  // of constants will be vectoried in SelectionDAG in
>> MergeConsecutiveStores.
>> +  if (VectorizableTree.size() < 3) {
>> +    if (!VectorizableTree.size()) {
>> +      assert(!ExternalUses.size() && "We should not have
any external
>> users");
>> +    }
>>      return 0;
>>    }
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130727/c4095ccc/attachment.html>