On Thu, Sep 23, 2021 at 8:32 AM Richard Biener via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Thu, 23 Sep 2021, Hongtao Liu wrote:
>
> > On Thu, Sep 23, 2021 at 9:48 AM Hongtao Liu <crazy...@gmail.com> wrote:
> > >
> > > On Wed, Sep 22, 2021 at 10:21 PM Martin Sebor <mse...@gmail.com> wrote:
> > > >
> > > > On 9/21/21 7:38 PM, Hongtao Liu wrote:
> > > > > On Mon, Sep 20, 2021 at 4:13 AM Martin Sebor <mse...@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].
Sorry I'm late to the party here...
Couldn't we run warnings earlier?
A while back, I converted the array bounds checker to the generic
range_query API, so it could be used either with the vr_values object
from the VRP pass, or with a ranger. There's no reason it should be
tied to VRP. We could make it its own pass and move it somewhere more
sensible.
Aldy
