[PATCH][LAA] Fix estimation of number of memchecks

[Adam Nemet]

> On Jun 2, 2015, at 12:02 PM, Adam Nemet <anemet at apple.com> wrote:
> 
> Hi,
> 
> The patch looks good but I’d like to see some benchmarking data before fully signing off on this.  Most importantly, do we vectorize more now?  My concern is that we may have tuned the threshold with the incorrect formula.

Also since we’re benchmarking in this area it would be great to completely move away from using an estimate for the number of memchecks.

There is a new API in LAA (RuntimePointerCheck::needsAnyChecking) that uses the actual number of memchecks needed.  Most notably it excludes checks that the dependence analysis deems unnecessary (i.e. the DependencySetId check in needsChecking).

We could add a new API returning the actual number instead of estimating this.

Adam

> 
> More comments below.  Also next time, please use Phab, thanks!
> 
> On Jun 2, 2015, at 9:44 AM, Silviu Baranga <silviu.baranga at arm.com <mailto:silviu.baranga at arm.com>> wrote:
>> 
>> Hi,
>>  
>> The attached patch fixes the estimation of the number of memchecks.
>>  
>> We need to add a runtime memcheck for pair of accesses (x,y) where at least one of x and y
>> are writes.
>>  
>> Assuming we have w writes and r reads, currently this number is  estimated as being
>> w* (w+r-1). This estimation will count (write,write) pairs twice and will overestimate
>> the number of checks required.
>>  
>> The actual number of memchecks is (r + w - 1) + (r + w - 2) + .. + r which is equal to
>> (w^2 + 2wr - w)/2.
>>  
>> The patch changes the formula and adds a small test.
>>  
>> Please review!
>>  
>> Thanks,
>> Silviu
>>  
>>  
>> diff --git a/lib/Analysis/LoopAccessAnalysis.cpp b/lib/Analysis/LoopAccessAnalysis.cpp
>> index 782bf70..5f97392 100644
>> --- a/lib/Analysis/LoopAccessAnalysis.cpp
>> +++ b/lib/Analysis/LoopAccessAnalysis.cpp
>> @@ -357,8 +357,12 @@ bool AccessAnalysis::canCheckPtrAtRT(
>>      if (IsDepCheckNeeded && CanDoRT && RunningDepId == 2)
>>        NumComparisons += 0; // Only one dependence set.
>>      else {
>> -      NumComparisons += (NumWritePtrChecks * (NumReadPtrChecks +
>> -                                              NumWritePtrChecks - 1));
>> +      // For r reads and w writes we will do
>> +      // (r + w - 1) + (r + w - 2) + .. + r checks,
>> +      // which evaluates to (w^2 + 2wr - w) / 2 checks.
>> +      NumComparisons += (NumWritePtrChecks * NumWritePtrChecks +
>> +                         2 * NumWritePtrChecks * NumReadPtrChecks -
>> +                         NumWritePtrChecks) / 2;
> 
> Please add more comment.  Clearly this is not obvious because whoever wrote the original code didn’t get it right.  Something like this: for each write we have to compare against all reads and write that we haven’t previously compared against.
> 
> You may also want to add the alternative way of how for example I worked out your formula:  it’s a combination of two writes plus all writes against all reads, i.e. C(w, 2) + w*r = w*(w-1)/2 + wr = (w^2 - w + 2wr)/2
> 
>>      }
>>  
>>      ++ASId;
>> diff --git a/test/Analysis/LoopAccessAnalysis/number-of-memchecks.ll b/test/Analysis/LoopAccessAnalysis/number-of-memchecks.ll
>> new file mode 100644
>> index 0000000..4ef3667
>> --- /dev/null
>> +++ b/test/Analysis/LoopAccessAnalysis/number-of-memchecks.ll
>> @@ -0,0 +1,52 @@
>> +; RUN: opt -loop-accesses -analyze < %s | FileCheck %s
>> +
>> +; 3 reads and 3 writes should need 12 memchecks
>> +
>> +target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
>> +target triple = "aarch64--linux-gnueabi"
>> +
>> +; CHECK: Memory dependences are safe with run-time checks
>> +; CHECK: 11:
>> +; CHECK-NOT: 12:
> 
> Your comment above and what you’re checking disagree.  I think the correct number is 12 and we used estimate 15 here.
> 
> Please also match the line "Run-time memory checks:” and then the number after it with CHECK-NEXT.
> 
> Thanks,
> Adam
> 
>> +
>> +define void @testf(i16* %a,
>> +               i16* %b,
>> +               i16* %c,
>> +               i16* %d,
>> +               i16* %e,
>> +               i16* %f) {
>> +entry:
>> +  br label %for.body
>> +
>> +for.body:                                         ; preds = %for.body, %entry
>> +  %ind = phi i64 [ 0, %entry ], [ %add, %for.body ]
>> +
>> +  %add = add nuw nsw i64 %ind, 1
>> +
>> +  %arrayidxA = getelementptr inbounds i16, i16* %a, i64 %ind
>> +  %loadA = load i16, i16* %arrayidxA, align 2
>> +
>> +  %arrayidxB = getelementptr inbounds i16, i16* %b, i64 %ind
>> +  %loadB = load i16, i16* %arrayidxB, align 2
>> +
>> +  %arrayidxC = getelementptr inbounds i16, i16* %c, i64 %ind
>> +  %loadC = load i16, i16* %arrayidxC, align 2
>> +
>> +  %mul = mul i16 %loadB, %loadA
>> +  %mul1 = mul i16 %mul, %loadC
>> +
>> +  %arrayidxD = getelementptr inbounds i16, i16* %d, i64 %ind
>> +  store i16 %mul1, i16* %arrayidxD, align 2
>> +
>> +  %arrayidxE = getelementptr inbounds i16, i16* %e, i64 %ind
>> +  store i16 %mul, i16* %arrayidxE, align 2
>> +
>> +  %arrayidxF = getelementptr inbounds i16, i16* %f, i64 %ind
>> +  store i16 %mul1, i16* %arrayidxF, align 2
>> +
>> +  %exitcond = icmp eq i64 %add, 20
>> +  br i1 %exitcond, label %for.end, label %for.body
>> +
>> +for.end:                                          ; preds = %for.body
>> +  ret void
>> +}
> 
> 
> 
>>  
>> <runtime.diff>_______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150602/370b1e5d/attachment.html>