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

[Martin Sebor <msebor@gmail.com>]

On 9/23/21 9:32 PM, Hongtao Liu wrote:
> 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?

Please don't add the -fno- option to the warning tests.  As I said,
I would prefer to either suppress the vectorization for the failing
cases by tweaking the test code or xfail them.  That way future
regressions won't be masked by the option.  Once we've moved
the warning to a more suitable pass we'll add a new test to verify
it works as intended or remove the xfails.

Thanks
Martin

>>>
>>> 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
> 
> 
>