[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.

[Daniel Berlin]

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>