[polly] Add diagnostic remark for ReportVariantBasePtr

[David Blaikie]

On Thu, Jun 26, 2014 at 2:13 PM, Tobias Grosser <tobias at grosser.es> wrote:
> On 26/06/2014 22:59, David Blaikie wrote:
>>
>> The current state is that just using -R should do the right thing
>> (include column info everywhere), but using it combined with any -g
>> will omit column info unless you request it explicitly
>> (-gcolumn-info).
>>
>> We're talking on llvm-dev about turning on column info by default so
>> that -R + -g results in the same diagnostic quality as -R alone.
>
>
> Sorry, the latest messages in this thread are inspired be the llvm-dev
> discussion. However, it seems that even with -gcolumn-info the loads and
> stores in the test case below have the same column info. My understanding is
> that clang even with -gcolumn-info does not create
> this information for each subexpression, right? Would you expect different
> column info for the load and store instructions?

Essentially there's no known answer here - no one's cared about column
info fidelity because no one uses it.

Certainly in your example it's not unreasonable to attribute the load
to the LHS and the store to the RHS (or maybe the '='). As we find
issues where column info has low fidelity that cause poor -R
diagnostic quality, we can fix them - though it'll be interesting to
keep an eye on the debug info size impact of this as it might creep up
as we start to produce finer grained line tables (and more IR
metadata, etc).

- David

>
> Here the test case:
>
>  struct b {
>    double **b;
>  };
>
>  void a(struct b *A, int p) {
>
>    for (int i=0; i<32; i++)
>      A->b[i] = A->b[i+p];   << -- Here
>  }
>
> here the output of 'clang -c  -O3  -Rpass-missed=polly -S -emit-llvm'
>
>   %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next.1, %for.body ]
>   %1 = add nsw i64 %indvars.iv, %0, !dbg !2
>   %2 = load double*** %b, align 8, !dbg !2, !tbaa !10
>   %arrayidx = getelementptr inbounds double** %2, i64 %1, !dbg !2
>   %3 = load double** %arrayidx, align 8, !dbg !2, !tbaa !15
>   %arrayidx3 = getelementptr inbounds double** %2, i64 %indvars.iv, !dbg !2
>   store double* %3, double** %arrayidx3, align 8, !dbg !2, !tbaa !15
>   %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1, !dbg !9
>   %4 = add nsw i64 %indvars.iv.next, %0, !dbg !2
>   %5 = load double*** %b, align 8, !dbg !2, !tbaa !10
>   %arrayidx.1 = getelementptr inbounds double** %5, i64 %4, !dbg !2
>   %6 = load double** %arrayidx.1, align 8, !dbg !2, !tbaa !15
>   %arrayidx3.1 = getelementptr inbounds double** %5, i64 %indvars.iv.next,
> !dbg !2
>   store double* %6, double** %arrayidx3.1, align 8, !dbg !2, !tbaa !15
>   %indvars.iv.next.1 = add nuw nsw i64 %indvars.iv.next, 1, !dbg !9
>   %exitcond.1 = icmp eq i64 %indvars.iv.next.1, 32, !dbg !9
>   br i1 %exitcond.1, label %for.end, label %for.body, !dbg !9
>
> all loads/stores have !dbg2
>
> With this clang hack:
>
>
> index 62ecc73..53c63f4 100644
> --- a/lib/CodeGen/CGExpr.cpp
> +++ b/lib/CodeGen/CGExpr.cpp
> @@ -771,6 +771,9 @@ LValue CodeGenFunction::EmitCheckedLValue(const Expr *E,
> TypeCheckKind TCK) {
>
>  /// length type, this is not possible.
>  ///
>  LValue CodeGenFunction::EmitLValue(const Expr *E) {
> +  if (CGDebugInfo *DI = getDebugInfo()) {
> +  DI->EmitLocation(Builder, E->getLocEnd(), true);
> +  }
>    switch (E->getStmtClass()) {
>    default: return EmitUnsupportedLValue(E, "l-value expression");
>
> We get:
>   %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next.1, %for.body ]
>   %1 = add nsw i64 %indvars.iv, %0, !dbg !10
>   %2 = load double*** %b, align 8, !dbg !2, !tbaa !11
>   %arrayidx = getelementptr inbounds double** %2, i64 %1, !dbg !2
>   %3 = load double** %arrayidx, align 8, !dbg !2, !tbaa !16
>   %arrayidx3 = getelementptr inbounds double** %2, i64 %indvars.iv, !dbg !17
>   store double* %3, double** %arrayidx3, align 8, !dbg !17, !tbaa !16
>   %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1, !dbg !9
>   %4 = add nsw i64 %indvars.iv.next, %0, !dbg !10
>   %5 = load double*** %b, align 8, !dbg !2, !tbaa !11
>   %arrayidx.1 = getelementptr inbounds double** %5, i64 %4, !dbg !2
>   %6 = load double** %arrayidx.1, align 8, !dbg !2, !tbaa !16
>   %arrayidx3.1 = getelementptr inbounds double** %5, i64 %indvars.iv.next,
> !dbg !17
>   store double* %6, double** %arrayidx3.1, align 8, !dbg !17, !tbaa !16
>   %indvars.iv.next.1 = add nuw nsw i64 %indvars.iv.next, 1, !dbg !9
>   %exitcond.1 = icmp eq i64 %indvars.iv.next.1, 32, !dbg !9
>   br i1 %exitcond.1, label %for.end, label %for.body, !dbg !9
>
> Now the store has !dbg !17 and the loads have !dbg !2
>
> This is enough for Polly reference individual errors with column carets
> (which was not working without the patch):
>
>
> /tmp/test.c:7:16: remark: The base address of this array is not invariant
> inside the loop [-Rpass-missed=polly-detect]
>      A->b[i] = A->b[i+p];
>                ^
> /tmp/test.c:7:6: remark: The base address of this array is not invariant
> inside the loop [-Rpass-missed=polly-detect]
>      A->b[i] = A->b[i+p];
>      ^
>
> I have no idea on which granularity clang is expected to provide column
> info. Has anybody an idea? Should clang emit column info for individual load
> instructions?
>
> Cheers,
> Tobias