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