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

[Hal Finkel]

----- Original Message -----
> From: "Nadav Rotem" <nrotem at apple.com>
> To: "Wei Mi" <wmi at google.com>
> Cc: reviews+D10445+public+d223e8b684afcd7b at reviews.llvm.org, "Commit Messages and Patches for LLVM"
> <llvm-commits at cs.uiuc.edu>, "David Li" <davidxl at google.com>
> Sent: Monday, June 15, 2015 11:23:47 AM
> Subject: Re: [PATCH] Choose the best consecutive candidate for a store	instruction in SLP vectorizer
> 
> 
> > On 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?
> 
> I usually run 3-5 iterations (using LNT), and this reduces the noise.
> Also, disabling turbo and hyperhreading helps.
>  
> The llvm test suite is excellent at catching correctness and
> performance regressions. I think it is a good idea to run the tests
> before commits like this.

Also, if you run with BENCHMARKING_ONLY defined, that should cut out most of the noisy ones.

 -Hal

> 
> > 
> >> 
> >> 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.
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory