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

[Hongtao Liu <crazylht@gmail.com>]

On Thu, Sep 23, 2021 at 11:18 PM Martin Sebor <msebor@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 <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.
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