[PATCH] Choose the best consecutive candidate for a store instruction in SLP vectorizer

[Xinliang David Li]

On Mon, Jun 15, 2015 at 9:20 AM, Wei Mi <wmi at google.com> wrote:
> On Mon, Jun 15, 2015 at 9:10 AM, Nadav Rotem <nrotem at apple.com> wrote:
>> Wei,
>>
>> Thanks for working on this.  Did you run the llvm test suite? Are there any performance wins or compile time regressions?
>
> Thanks for the quick review.
> I didn't run llvm testsuite because I found there were some noisy
> benchmarks last time when I run it. Is there a good way to filter
> those noisy benchmarks?

The performance test should be run regardless. The results from noisy
benchmarks can be marked (run them more times to see if the diff is
statistically significant. Disabling frequency scaling, address
randomization etc).

David

>
>>
>> I have a few comments below:
>>
>> -    for (unsigned j = 0; j < e; ++j) {
>> -      if (i == j)
>> -        continue;
>> -      const DataLayout &DL = Stores[i]->getModule()->getDataLayout();
>> +    const DataLayout &DL = Stores[i]->getModule()->getDataLayout();
>>
>> Please initialize ‘j' when you declare it. Also, why unsigned?
>
> Because e is unsigned, there will be a warning for j < e if j is not unsigned.
>
>>
>> +    unsigned j;
>> +    // If a store has multiple consectutive store candidates, choose
>> +    // the immediate succeeding or preceding one.
>> +    for (j = i + 1; j < e; ++j) {
>> +      if (R.isConsecutiveAccess(Stores[i], Stores[j], DL)) {
>> +        Tails.insert(Stores[j]);
>> +        Heads.insert(Stores[i]);
>> +        ConsecutiveChain[Stores[i]] = Stores[j];
>> +        break;
>> +      }
>> +    }
>>
>> Please document this line, or simplify it.
>>
>> +    if (j < e)
>> +      continue;
>>
>> At this point you are defining a new J variable, with a different type. This is confusing.
>>
>> +    for (int j = i - 1; j >= 0; --j) {
>>
>>
>> Can you think of a way to write this code without code duplication?
>>
>
> I will find a way to remove the duplication.
>
> Thanks,
> Wei.