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.
> 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.
> 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?
> >>- 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.
> 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!
Thanks,
Segher
