Re: [PATCH] rs6000/test: Adjust some cases due to O2 vect [PR102658]

[Hongtao Liu <crazylht@gmail.com>]

On Tue, Oct 12, 2021 at 11:49 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 10/11/21 8:31 PM, Hongtao Liu wrote:
> > On Tue, Oct 12, 2021 at 4:08 AM Martin Sebor via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> On 10/11/21 11:43 AM, Segher Boessenkool wrote:
> >>> On Mon, Oct 11, 2021 at 10:23:03AM -0600, Martin Sebor wrote:
> >>>> On 10/11/21 9:30 AM, Segher Boessenkool wrote:
> >>>>> On Mon, Oct 11, 2021 at 10:47:00AM +0800, Kewen.Lin wrote:
> >>>>>> - For generic test cases, it follows the existing suggested
> >>>>>> practice with necessary target/xfail selector.
> >>>>>
> >>>>> Not such a great choice.  Many of those tests do not make sense with
> >>>>> vectorisation enabled.  This should have been thought about, in some
> >>>>> cases resulting in not running the test with vectorisation enabled, and
> >>>>> in some cases duplicating the test, once with and once without
> >>>>> vectorisation.
> >>>>
> >>>> The tests detect bugs that are present both with and without
> >>>> vetctorization, so they should pass both ways.
> >>>
> >>> Then it should be tested both ways!  This is my point.
> >>
> >> Agreed.  (Most warnings are tested with just one set of options,
> >> but it's becoming apparent that the middle end ones should be
> >> exercised more extensively.)
> >>
> >>>
> >>>> That they don't
> >>>> tells us that that the warnings need work (they were written with
> >>>> an assumption that doesn't hold anymore).
> >>>
> >>> They were written in world A.  In world B many things behave
> >>> differently.  Transplanting the testcases from A to B without any extra
> >>> analysis will not test what the testcases wanted to test, and possibly
> >>> nothing at all anymore.
> >>
> >> Absolutely.
> >>
> >>>
> >>>> We need to track that
> >>>> work somehow, but simply xfailing them without making a record
> >>>> of what underlying problem the xfails correspond to isn't the best
> >>>> way.  In my experience, what works well is opening a bug for each
> >>>> distinct limitation (if one doesn't already exist) and adding
> >>>> a reference to it as a comment to the xfail.
> >>>
> >>> Probably, yes.
> >>>
> >>>>> But you are just following established practice, so :-)
> >>>
> >>> I also am okay with this.  If it was decided x86 does not have to deal
> >>> with these (generic!) problems, then why should we do other people's
> >>> work?
> >>
> >> I don't know that anything was decided.  I think those changes
> >> were made in haste, and (as you noted in your review of these
> >> updates to them), were incomplete (missing comments referencing
> >> the underlying bugs or limitations).  Now that we've noticed it
> >> we should try to fix it.  I'm not expecting you (or Kwen) to do
> >> other people's work, but it would help to let them/us know that
> >> there is work for us to do.  I only noticed the problem by luck.
> >>
> >>>>>> -  struct A1 a = { 0, { 1 } };   // { dg-warning
> >>>>>> "\\\[-Wstringop-overflow" "" { target { i?86-*-* x86_64-*-* } } }
> >>>>>> +  struct A1 a = { 0, { 1 } };   // { dg-warning
> >>>>>> "\\\[-Wstringop-overflow" "" { target { i?86-*-* x86_64-*-* powerpc*-*-*
> >>>>>> } } }
> >>>>
> >>>> As I mentioned in the bug, when adding xfails for regressions
> >>>> please be sure to reference the bug that tracks the underlying
> >>>> root cause.]
> >>>
> >>> You are saying this to whoever added that x86 xfail I hope.
> >>
> >> In general it's an appeal to both authors and reviewers of such
> >> changes.  Here, it's mostly for Hongtao who apparently added all
> >> these undocumented xfails.
> >>
> >>>> There may be multiple problems, and we need to
> >>>> identify what it is in each instance.  As the author of
> >>>> the tests I can help with that but not if I'm not in the loop
> >>>> on these changes (it would seem prudent to get the author's
> >>>> thoughts on such sweeping changes to their work).
> >>>
> >>> Yup.
> >>>
> >>>> I discussed one of these failures with Hongtao in detail at
> >>>> the time autovectorization was being enabled and made the same
> >>>> request then but I didn't realize the problem was so pervasive.
> >>>>
> >>>> In addition, the target-specific conditionals in the xfails are
> >>>> going to be difficult to maintain.
> >>>
> >>> It is a cop-out.  Especially because it makes no comment why it is
> >>> xfailed (which should *always* be explained!)
> >>>
> >>>> It might be okay for one or
> >>>> two in a single test but for so many we need a better solution
> >>>> than that.  If autovectorization is only enabled for a subset
> >>>> of targets then a solution might be to add a new DejagGNU test
> >>>> for it and conditionalize the xfails on it.
> >>>
> >>> That, combined with duplicating these tests and still testing the
> >>> -fno-vectorization situation properly.  Those tests tested something.
> >>> With vectorisation enabled they might no longer test that same thing,
> >>> especially if the test fails now!
> >>
> >> Right.  The original autovectorization change was made either
> >> without a full analysis of its impact on the affected warnings,
> >> or its impact wasn't adequately captured (either in the xfails
> >> comments or by opening bugs for them).  Now that we know about
> >> this we should try to fix it.  The first step toward that is
> >> to review the xfailed test cases and for each add a comment with
> >> the bug that captures its root cause.
> >>
> >> Hongtao, please let me know if you are going to work on that.
> > I will make a copy of the tests to test the -fno-tree-vectorize
> > scenario(the original test).
> > For the xfails, they're analyzed and recorded in pr102462/pr102697,
> > sorry for not adding comments to them.
>
> Thanks for raising pr102697!  It captures the essence of the bug
> that's masked by the vectorization of the invalid store.  This is
> due to the hack I pointed to in the discussion below:
>
> https://gcc.gnu.org/pipermail/gcc-patches/2021-September/580172.html
>
> > The root causes for those xfails are divided into 2 categories:
> >
> > 1. All accesses are out of bound, and after vectorization, there are
> > some warnings missing.(Because there is only 1 access after
> > vectorization, 2 accesses w/o vectorization, and diagnostic is based
> > on access).
>
> If these involve -Wstringop-overflow for accesses that span
> multiple subobjects, as in writing past the end of one member
> and over the following member, then that would be due to
> pr102697 (the hack above).
>
> > 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.
>
> Right, this is the issue we talked about during the review of
> your patch, and I think is captured in the test case in comment
> #4 on pr102462.
>
> >
> > for pr102697, it looks like the testcase is not well written.
>
> The test case is correct.  I've added my comments to the PR
> and confirmed it as a GCC 12 regression.  (I may not have
> the time to fix it for GCC 12 but I will plan to get to it
> for GCC 13 unless someone beats me to it.)
>
> I think it might be helpful to open a bug just for case (2)
> and reference it in all the corresponding xfails.
>
> pr102462 talks about three distinct cases and mentions
> -Warray-bounds as well as -Wstringop-overflow.  It's not clear
> from it exactly which of the three cases it's meant to be about.
>
> There is also an undocumented xfail in
> g++.dg/warn/Wuninitialized-13.C.  It should get its own bug
> even if the essence of the problem is the same (the warning
> doesn't share an implementation with -Warray-bounds or
> -Wstringop-overflow so a fix will most likely need to be
> separate from one for the other bugs).
>
> Coming back to the xfail conditionals, do you think you'll
> be able to put together some target-supports magic so they
> don't have to enumerate all the affected targets?
>
Those failure testcases(exposed by x86 part)can be extracted and
categorized into 3 below testcases.
Question is can we check vectorization ability in
dg-require-effective-target for those testcase?
If we can, we  can dynamically check whether each target supports this xfail.

foo is used for vector(2) char, foo1 vector(4) char, foo2 vector(2) short.

char p[4] __attribute__ ((aligned (4)));
void
foo ()
{
  p[0] = 1;
  p[1] = 2;
  p[2] = 3;
}

void
foo1 ()
{
  p[0] = 0;
  p[1] = 1;
  p[2] = 2;
  p[3] = 3;
}

void
foo2 (short* q)
{
  q[0] = 0;
  q[1] = 1;
}
> Martin




--
BR,
Hongtao