On 3/3/21 3:31 AM, Richard Biener wrote:
On Tue, Mar 2, 2021 at 9:23 PM Martin Sebor <mse...@gmail.com> wrote:
On 3/2/21 3:39 AM, Richard Biener wrote:
On Fri, Jan 22, 2021 at 12:39 AM Martin Sebor via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
The hack I put in compute_objsize() last January for pr93200 isn't
quite correct. It happened to suppress the false positive there
but, due to what looks like a thinko on my part, not in some other
test cases involving vectorized stores.
The attached change adjusts the hack to have compute_objsize() give
up on all MEM_REFs with a vector type. This effectively disables
-Wstringop-{overflow,overread} for vector accesses (either written
by the user or synthesized by GCC from ordinary accesses). It
doesn't affect -Warray-bounds because this warning doesn't use
compute_objsize() yet. When it does (it should considerably
simplify the code) some additional changes will be needed to
preserve -Warray-bounds for out of bounds vector accesses.
The test this patch adds should serve as a reminder to make
it if we forget.
Tested on x86_64-linux. Since PR 94655 was reported against GCC
10 I'd like to apply this fix to both the trunk and the 10 branch.
The proposed code reads even more confusingly than before.
What is called 'ptr' is treated as a reference and what you
then name ptrtype is the type of the reference.
That leads to code like
+ if (code == MEM_REF && TREE_CODE (ptrtype) == VECTOR_TYPE)
what? the pointer type is a VECTOR_TYPE?!
I think you are looking for whether the reference type is a VECTOR_TYPE.
Part of the confusion is likely because the code commons
if (code == ARRAY_REF || code == MEM_REF)
but in one case operand zero is a pointer to the object (MEM_REF) and
in one case
it is the object (ARRAY_REF).
Please refactor this code so one can actually read it literally
without second-guessing proper variable names.
I agree that handling both REFs in the same block makes the code
more difficult to follow than it needs to be.
In the attached patch I've factored the code out into two helper
functions, one for ARRAY_REF and another for MEM_REF, and chosen
(hopefully) clearer names for the local variables.
A similar refactoring could be done for COMPONENT_REF and also
for SSA_NAME to reduce the size of the function and make it much
easier to follow. I stopped short of doing that but I can do it
for GCC 12 when I move the whole compute_objsize() implementation
into a more appropriate place (e.g., its own file).
+ if (!addr && TREE_CODE (TREE_TYPE (reftype)) == POINTER_TYPE)
POINTER_TYPE_P ()
I think this is intentional: POINTER_TYPE_P() includes C++ references
and although they don't come up here (there's no such thing as arrays
of references) it didn't seem appropriate to be including them in
the test even if it's benign.
+ /* Avoid arrays of pointers. FIXME: Hande pointers to arrays
+ of known bound. */
+ return false;
+ if (TREE_CODE (TREE_TYPE (mref)) == VECTOR_TYPE)
+ {
+ /* Hack: Give up for MEM_REFs of vector types; those may be
+ synthesized from multiple assignments to consecutive data
+ members (see PR 93200 and 96963).
VECTOR_TYPE_P (TREE_TYPE (mref))
Okay.
+ 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).
You could gate this on cfun->curr_properties & PROP_gimple_lvec
which is only set after vectorization. Users can write vector
accesses using intrinsics or GCCs vector extension and I guess
warning for those is useful (and they less likely will cover multiple
fields).
Thanks, that's useful to know! I'll keep it in mind for GCC 12.
Note the vectorizer also uses integer types for vector accesses
either when vectorizing using 'generic' vectors (for loads, stores
and bit operations we don't need any vector ISA) or when
composing vectors.
Note store-merging has the same issue (but fortunately runs later?)
Yes, it's the same issue (as is FRE/PRE using one member to write
to the next) and it does cause false positives depending on where
the code is called from. They're on my to do list for GCC 12 to
avoid as we discussed, by transform those into MEM_REFs with
the enclosing object + offset instead of the member.
OK with the above changes.
I've committed the patch without the POINTER_TYPE_P change (I'm
okay with it if you feel it necessary.)
Martin
Thanks,
Richard.
Martin
Thanks,
Richard.
Martin
