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.
Martin
