Re: [PATCH] correct fix to avoid false positives for vectorized stores (PR 96963, 94655)

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Mar 2, 2021 at 9:23 PM Martin Sebor <msebor@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 ()

+    /* 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))

+        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).

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

OK with the above changes.

Thanks,
Richard.

> Martin
>
> >
> > Thanks,
> > Richard.
> >
> >
> >
> >> Martin
>