Re: [PATCH] Enable auto-vectorization at O2 with very-cheap cost model.

[Richard Biener <rguenther@suse.de>]

On Thu, 23 Sep 2021, Hongtao Liu wrote:

> On Thu, Sep 23, 2021 at 9:48 AM Hongtao Liu <crazylht@gmail.com> wrote:
> >
> > On Wed, Sep 22, 2021 at 10:21 PM Martin Sebor <msebor@gmail.com> wrote:
> > >
> > > On 9/21/21 7:38 PM, Hongtao Liu wrote:
> > > > On Mon, Sep 20, 2021 at 4:13 AM Martin Sebor <msebor@gmail.com> wrote:
> > > ...
> > > >>>>> diff --git a/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c b/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c
> > > >>>>> index 1d79930cd58..9351f7e7a1a 100644
> > > >>>>> --- a/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c
> > > >>>>> +++ b/gcc/testsuite/c-c++-common/Wstringop-overflow-2.c
> > > >>>>> @@ -1,7 +1,7 @@
> > > >>>>>    /* PR middle-end/91458 - inconsistent warning for writing past the end
> > > >>>>>       of an array member
> > > >>>>>       { dg-do compile }
> > > >>>>> -   { dg-options "-O2 -Wall -Wno-array-bounds -fno-ipa-icf" } */
> > > >>>>> +   { dg-options "-O2 -Wall -Wno-array-bounds -fno-ipa-icf -fno-tree-vectorize" } */
> > > >>>>
> > > >>>> The testcase is large - what part requires this change?  Given the
> > > >>>> testcase was added for inconsistent warnings do they now become
> > > >>>> inconsistent again as we enable vectorization at -O2?
> > > >>>>
> > > >>>> That said, the testcase adjustments need some explaining - I suppose
> > > >>>> you didn't just slap -fno-tree-vectorize to all of those changing
> > > >>>> behavior?
> > > >>>>
> > > >>> void ga1_ (void)
> > > >>> {
> > > >>>     a1_.a[0] = 0;
> > > >>>     a1_.a[1] = 1;                 // { dg-warning "\\\[-Wstringop-overflow" }
> > > >>>     a1_.a[2] = 2;                 // { dg-warning "\\\[-Wstringop-overflow" }
> > > >>>
> > > >>>     struct A1 a;
> > > >>>     a.a[0] = 0;
> > > >>>     a.a[1] = 1;                   // { dg-warning "\\\[-Wstringop-overflow" }
> > > >>>     a.a[2] = 2;                   // { dg-warning "\\\[-Wstringop-overflow" }
> > > >>>     sink (&a);
> > > >>> }
> > > >>>
> > > >>> It's supposed to be 2 warning for a.a[1] = 1 and a.a[2] = 1 since
> > > >>> there are 2 accesses, but after enabling vectorization, there's only
> > > >>> one access, so one warning is missing which causes the failure.
> > >
> > > With the stores vectorized, is the warning on the correct line or
> > > does it point to the first store, the one that's in bounds, as
> > > it does with -O3?  The latter would be a regression at -O2.
> > For the upper case, It points to the second store which is out of
> > bounds, the third store warning is missing.
> > >
> > > >>
> > > >> I would find it preferable to change the test code over disabling
> > > >> optimizations that are on by default.  My concern is that the test
> > > >> would no longer exercise the default behavior.  (The same goes for
> > > >> the -fno-ipa-icf option.)
> > > > Hmm, it's a middle-end test, for some backend, it may not do
> > > > vectorization(it depends on TARGET_VECTOR_MODE_SUPPORTED_P and
> > > > relative cost model).
> > >
> > > Yes, there are quite a few warning tests like that.  Their main
> > > purpose is to verify that in common GCC invocations (i.e., without
> > > any special options) warnings are a) issued when expected and b)
> > > not issued when not expected.  Otherwise, middle end warnings are
> > > known to have both false positives and false negatives in some
> > > invocations, depending on what optimizations are in effect.
> > > Indiscriminately disabling common optimizations for these large
> > > tests and invoking them under artificial conditions would
> > > compromise this goal and hide the problems.
> > >
> > > If enabling vectorization at -O2 causes regressions in the quality
> > > of diagnostics (as the test failure above indicates seems to be
> > > happening) we should investigate these and open bugs for them so
> > > they can be fixed.  We can then tweak the specific failing test
> > > cases to avoid the failures until they are fixed.
> > There are indeed cases of false positives and false negatives
> > .i.e.
> > // Verify warning for access to a definition with an initializer that
> > // initializes the one-element array member.
> > struct A1 a1i_1 = { 0, { 1 } };
> >
> > void ga1i_1 (void)
> > {
> >   a1i_1.a[0] = 0;
> >   a1i_1.a[1] = 1;               // { dg-warning "\\\[-Wstringop-overflow" }
> >   a1i_1.a[2] = 2;               // { dg-warning "\\\[-Wstringop-overflow" }
> >
> >   struct A1 a = { 0, { 1 } }; --- false positive here.
> >   a.a[0] = 1;
> >   a.a[1] = 2;                   // { dg-warning
> > "\\\[-Wstringop-overflow" } false negative here.
> >   a.a[2] = 3;                   // { dg-warning
> > "\\\[-Wstringop-overflow" } false negative here.
> >   sink (&a);
> > }
> Similar for
> * gcc.dg/Warray-bounds-51.c.
> * gcc.dg/Warray-parameter-3.c
> * gcc.dg/Wstringop-overflow-14.c
> * gcc.dg/Wstringop-overflow-21.c
> 
> So there're 3 situations.
> 1. All accesses are out of bound, and after vectorization, there are
> some warnings missing.
> 2. Part of accesses are inbound, part of accesses are out of bound,
> and after vectorization, the warning goes from out of bound line to
> inbound line.
> 3. All access are out of bound, and after vectoriation, all warning
> are missing, and goes to a false-positive line.

I remember some of the warning code explicitely excuses itself from
even trying to deal with vectorized loads/stores, that might need to
be revisited.  It would also be useful to verify whether the line
info on the vectorized loads/stores is sensible (if you dump with
-lineno you get stmts with line numbers).

It is of course impossible to preserve the location of all original
scalar accesses exactly - it _might_ be possible to create a new
location range (not sure how they are exactly represented), but then
if we vectorize say

  a[0] = b[0];
  a[1] = b[1];

we'd somehow get a multi-line location range that covers unrelated
bits (the intermediate load or store).

So currently the vectorizer simply chooses the location of one of
the scalar loads or stores for the vectorized access (and it might do
so quite randomly).

Note I don't think it's feasible to not vectorize out-of-bound loads
or stores for the same reason that you'll say you can't do the
warnings all before vectorizing because the might expose themselves
only later.

So we simply have to cope with the reality that GCC is optimizing and
that the later we perform analysis for warnings the more the original
code is mangled.  As we moved array-bound warnings before unrolling
so we should move this particular warning to before vectorization
[loop optimization].

Richard.