On Thu, Sep 23, 2021 at 11:18 PM Martin Sebor <mse...@gmail.com> wrote:
>
> On 9/23/21 12:30 AM, Richard Biener 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.
My mistake, there's no case3, just case 1 and case2.
So i'm going to install the patch, ok?
> >
> > 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).
>
> Yes, it's this code in handle_mem_ref() in pointer-query.cc:
>
> if (VECTOR_TYPE_P (TREE_TYPE (mref)))
> {
> /* Hack: Handle MEM_REFs of vector types as those to complete
> objects; those may be synthesized from multiple assignments
> to consecutive data members (see PR 93200 and 96963).
> FIXME: Vectorized assignments should only be present after
> vectorization so this hack is only necessary after it has
> run and could be avoided in calls from prior passes (e.g.,
> tree-ssa-strlen.c).
> FIXME: Deal with this more generally, e.g., by marking up
> such MEM_REFs at the time they're created. */
> ostype = 0;
> }
>
> This code is used by both -Warray-bounds and -Wstringop-overflow
> but because the former runs before vectorization the former isn't
> affected by the auto-vectorization change.
>
> >
> > 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).
>
> This sounds like an interesting idea to explore, though I'm not
> sure what we'd end up with if the stores were discontiguous (i.e.,
> had unrelated statements in between).
>
> >
> > 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.
>
> Sure, but it would be helpful to at least avoid vectorizing (as
> well as store merging) the cases where the out-of-bounds access
> is easily detectable. As it is, the code I looked at makes no
> effort to check.
>
> >
> > 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].
>
> That makes sense. See also my comment 7 on bug 102462 Hongtao
> opened for this problem. But just to be clear: I expect moving
> these warnings won't be entirely straightforward.
>
> Martin
--
BR,
Hongtao
