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

[Martin Sebor <msebor@gmail.com>]

On 10/11/21 9:30 AM, Segher Boessenkool wrote:
> Hi!
> 
> On Mon, Oct 11, 2021 at 10:47:00AM +0800, Kewen.Lin wrote:
>> As PR102658 shows, commit r12-4240 enables vectorization at O2,
>> some cases need to be adjusted accordingly for rs6000 port.
>>
>> - For target specific test cases, this adds -fno-tree-vectorize
>> to retain original test points, otherwise vectorization can
>> make some expected scalar instructions gone or generate some
>> unexpected instructions for vector construction.
> 
> Ah good choice.
> 
>> - 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.  That they don't
tells us that that the warnings need work (they were written with
an assumption that doesn't hold 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.

> 
> But you are just following established practice, so :-)
> 
>> -  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.  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).

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

Martin

> 
> I don't know if powerpc*-*-* is the correct choice in all these cases.
> Sometimes it might have to be powerpc*-*-linux* or similar.  We'll find
> out :-)
> 
> (An xfail causes XPASS if the test does *not* fail).
> 
>> +/* Now O2 enables vectorization by default, which generates unexpected float
>> +   conversion for vector construction, so simply disable it.  */
> 
> It is good to see these comments.  I love puzzles, but not in the
> testsuite! :-)
> 
> Okay for trunk.  Thanks!
> 
> 
> Segher
>