Re: [PATCH] -fopt-info: add indentation via DUMP_VECT_SCOPE

[David Malcolm <dmalcolm@redhat.com>]

On Mon, 2018-07-02 at 14:23 +0200, Christophe Lyon wrote:
> On Fri, 29 Jun 2018 at 10:09, Richard Biener <richard.guenther@gmail.
> com> wrote:
> > 
> > On Tue, Jun 26, 2018 at 5:43 PM David Malcolm <dmalcolm@redhat.com>
> > wrote:
> > > 
> > > This patch adds a concept of nested "scopes" to dumpfile.c's
> > > dump_*_loc
> > > calls, and wires it up to the DUMP_VECT_SCOPE macro in tree-
> > > vectorizer.h,
> > > so that the nested structure is shown in -fopt-info by
> > > indentation.
> > > 
> > > For example, this converts -fopt-info-all e.g. from:
> > > 
> > > test.c:8:3: note: === analyzing loop ===
> > > test.c:8:3: note: === analyze_loop_nest ===
> > > test.c:8:3: note: === vect_analyze_loop_form ===
> > > test.c:8:3: note: === get_loop_niters ===
> > > test.c:8:3: note: symbolic number of iterations is (unsigned int)
> > > n_9(D)
> > > test.c:8:3: note: not vectorized: loop contains function calls or
> > > data references that cannot be analyzed
> > > test.c:8:3: note: vectorized 0 loops in function
> > > 
> > > to:
> > > 
> > > test.c:8:3: note: === analyzing loop ===
> > > test.c:8:3: note:  === analyze_loop_nest ===
> > > test.c:8:3: note:   === vect_analyze_loop_form ===
> > > test.c:8:3: note:    === get_loop_niters ===
> > > test.c:8:3: note:   symbolic number of iterations is (unsigned
> > > int) n_9(D)
> > > test.c:8:3: note:   not vectorized: loop contains function calls
> > > or data references that cannot be analyzed
> > > test.c:8:3: note: vectorized 0 loops in function
> > > 
> > > showing that the "symbolic number of iterations" message is
> > > within
> > > the "=== analyze_loop_nest ===" (and not within the
> > > "=== vect_analyze_loop_form ===").
> > > 
> > > This is also enabling work for followups involving optimization
> > > records
> > > (allowing the records to directly capture the nested structure of
> > > the
> > > dump messages).
> > > 
> > > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> > > 
> > > OK for trunk?
> 
> Hi,
> 
> I've noticed that this patch (r262246) caused regressions on aarch64:
>     gcc.dg/vect/slp-perm-1.c -flto -ffat-lto-objects  scan-tree-dump
> vect "note: Built SLP cancelled: can use load/store-lanes"
>     gcc.dg/vect/slp-perm-1.c scan-tree-dump vect "note: Built SLP
> cancelled: can use load/store-lanes"
>     gcc.dg/vect/slp-perm-2.c -flto -ffat-lto-objects  scan-tree-dump
> vect "note: Built SLP cancelled: can use load/store-lanes"
>     gcc.dg/vect/slp-perm-2.c scan-tree-dump vect "note: Built SLP
> cancelled: can use load/store-lanes"
>     gcc.dg/vect/slp-perm-3.c -flto -ffat-lto-objects  scan-tree-dump
> vect "note: Built SLP cancelled: can use load/store-lanes"
>     gcc.dg/vect/slp-perm-3.c scan-tree-dump vect "note: Built SLP
> cancelled: can use load/store-lanes"
>     gcc.dg/vect/slp-perm-5.c -flto -ffat-lto-objects  scan-tree-dump
> vect "note: Built SLP cancelled: can use load/store-lanes"
>     gcc.dg/vect/slp-perm-5.c scan-tree-dump vect "note: Built SLP
> cancelled: can use load/store-lanes"
>     gcc.dg/vect/slp-perm-6.c -flto -ffat-lto-objects  scan-tree-dump
> vect "note: Built SLP cancelled: can use load/store-lanes"
>     gcc.dg/vect/slp-perm-6.c scan-tree-dump vect "note: Built SLP
> cancelled: can use load/store-lanes"
>     gcc.dg/vect/slp-perm-7.c -flto -ffat-lto-objects  scan-tree-dump
> vect "note: Built SLP cancelled: can use load/store-lanes"
>     gcc.dg/vect/slp-perm-7.c scan-tree-dump vect "note: Built SLP
> cancelled: can use load/store-lanes"
>     gcc.dg/vect/slp-perm-8.c -flto -ffat-lto-objects  scan-tree-dump
> vect "note: Built SLP cancelled: can use load/store-lanes"
>     gcc.dg/vect/slp-perm-8.c scan-tree-dump vect "note: Built SLP
> cancelled: can use load/store-lanes"
> 
> The problem is that now there are more spaces between "note:" and
> "Built", the attached small patch does that for slp-perm-1.c.

Sorry about the breakage.

> Is it the right way of fixing it or do we want to accept any amount
> of
> spaces for instance?

I don't think we want to hardcode the amount of space in the dumpfile. 
The idea of my patch was to make the dump more human-readable (I hope)
by visualizing the nesting structure of the dump messages, but I think
we shouldn't "bake" that into the expected strings, as someone might
want to add an intermediate nesting level.

Do we really need to look for the "note:" in the scan-tree-dump? 
Should that directive be rewritten to:

-/* { dg-final { scan-tree-dump "note: Built SLP cancelled: can use load/store-lanes" "vect" { target { vect_perm3_int && vect_load_lanes } } } } */
+/* { dg-final { scan-tree-dump "Built SLP cancelled: can use load/store-lanes" "vect" { target { vect_perm3_int && vect_load_lanes } } } } */

which I believe would match any amount of spaces.

Alternatively a regex accepting any amount of space ought to work, if
we care that the message begins with "note: ".

The "note: " comes from dumpfile.c's dump_loc, and is emitted
regardless of whether it's a MSG_NOTE vs a MSG_MISSED_OPTIMIZATION or
whatever.  Given that, maybe we should just drop the "note: " prefix
from these scan-tree-dump expected regexes?  (assuming that works)

> I'm surprised there is such little impact on the testsuite though.

I see lots of scan-tree-dump* directives in the vect part of the
testsuite, but it seems that only these ones use the "note: " prefix; I
think everything else was matching against the message whilst ignoring
the prefix, so it didn't matter when the prefix changed
(I double-checked and these scan-tree-dump directives didn't trigger on
my x86_64 testing of the patch, due to { target { vect_perm3_int &&
vect_load_lanes } }, where I see
check_effective_target_vect_load_lanes: returning 0 in the log)

> If OK, I'll update the patch to take the other slp-perm-[235678].c
> tests into account.
> 
> Thanks,
> 
> Christophe

Sorry again about the breakage.
Dave