[v2,rs6000] Put dg-options before effective target checks

  Hi,
  This patch changes the sequence of test directives for 3 test cases.
Originally, these 3 cases got failed or unsupported on some platforms, as
their effective target checks depend on compiling options.

  Bootstrapped and tested on powerpc64-linux BE and LE with no regressions.
Is this okay for trunk? Any recommendations? Thanks a lot.

Thanks
Gui Haochen

ChangeLog
2022-08-31  Haochen Gui  <guihaoc@linux.ibm.com>

rs6000: Change the sequence of test directives for some test cases.  Put
dg-options before effective target checks as those has_arch_* adopt
current_compiler_flags in their checks and rely on compiling options to get an
accurate check.  dg-options setting before dg-require-effective-target are
added into current_compiler_flags, but not added if they're after.  So
adjusting the location of dg-options makes the check more robust.

gcc/testsuite/
	* gcc.target/powerpc/pr92398.p9+.c: Put dg-options before effective
	target check.  Replace lp64 check with has_arch_ppc64 and int128.
	* gcc.target/powerpc/pr92398.p9-.c: Likewise.
	* gcc.target/powerpc/pr93453-1.c: Put dg-options before effective
	target check.


patch.diff
  

Hi Haochen,

on 2022/9/1 13:30, HAO CHEN GUI wrote:
> Hi,
>   This patch changes the sequence of test directives for 3 test cases.
> Originally, these 3 cases got failed or unsupported on some platforms, as
> their effective target checks depend on compiling options.
> 

Thanks for the updated patch!

I just found that it seems all the three test cases suffer the empty
TU error issue from those has_arch* effective target checks?

If yes, it looks we don't need to bother this once patch [1] gets
landed?

Sorry, I didn't notice and ask when reviewing the previous version.

[1] https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598748.html

BR,
Kewen

>   Bootstrapped and tested on powerpc64-linux BE and LE with no regressions.
> Is this okay for trunk? Any recommendations? Thanks a lot.
> 
> Thanks
> Gui Haochen
> 
> ChangeLog
> 2022-08-31  Haochen Gui  <guihaoc@linux.ibm.com>
> 
> rs6000: Change the sequence of test directives for some test cases.  Put
> dg-options before effective target checks as those has_arch_* adopt
> current_compiler_flags in their checks and rely on compiling options to get an
> accurate check.  dg-options setting before dg-require-effective-target are
> added into current_compiler_flags, but not added if they're after.  So
> adjusting the location of dg-options makes the check more robust.
> 
> gcc/testsuite/
> 	* gcc.target/powerpc/pr92398.p9+.c: Put dg-options before effective
> 	target check.  Replace lp64 check with has_arch_ppc64 and int128.
> 	* gcc.target/powerpc/pr92398.p9-.c: Likewise.
> 	* gcc.target/powerpc/pr93453-1.c: Put dg-options before effective
> 	target check.
> 
> 
> patch.diff
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr92398.p9+.c b/gcc/testsuite/gcc.target/powerpc/pr92398.p9+.c
> index 72dd1d9a274..b4f5c7f4b82 100644
> --- a/gcc/testsuite/gcc.target/powerpc/pr92398.p9+.c
> +++ b/gcc/testsuite/gcc.target/powerpc/pr92398.p9+.c
> @@ -1,6 +1,10 @@
> -/* { dg-do compile { target { lp64 && has_arch_pwr9 } } } */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power9 -mvsx" } */
> +/* { dg-require-effective-target has_arch_ppc64 } */
> +/* { dg-require-effective-target int128 } */
>  /* { dg-require-effective-target powerpc_vsx_ok } */
> -/* { dg-options "-O2 -mvsx" } */
> +/* The test case can be compiled on all platforms with compiling option
> +   -mdejagnu-cpu=power9.  */
> 
>  /* { dg-final { scan-assembler-times {\mmtvsrdd\M} 1 } } */
>  /* { dg-final { scan-assembler-times {\mxxlnor\M} 1 } } */
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr92398.p9-.c b/gcc/testsuite/gcc.target/powerpc/pr92398.p9-.c
> index bd7fa98af51..4e6a8c8cb8e 100644
> --- a/gcc/testsuite/gcc.target/powerpc/pr92398.p9-.c
> +++ b/gcc/testsuite/gcc.target/powerpc/pr92398.p9-.c
> @@ -1,6 +1,8 @@
> -/* { dg-do compile { target { lp64 && {! has_arch_pwr9} } } } */
> -/* { dg-require-effective-target powerpc_vsx_ok } */
>  /* { dg-options "-O2 -mvsx" } */
> +/* { dg-do compile { target { ! has_arch_pwr9 } } } */
> +/* { dg-require-effective-target int128 } */
> +/* { dg-require-effective-target has_arch_ppc64 } */
> +/* { dg-require-effective-target powerpc_vsx_ok } */
> 
>  /* { dg-final { scan-assembler-times {\mnot\M} 2 { xfail be } } } */
>  /* { dg-final { scan-assembler-times {\mstd\M} 2 { xfail { { {! has_arch_pwr9} && has_arch_pwr8 } && be } } } } */
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr93453-1.c b/gcc/testsuite/gcc.target/powerpc/pr93453-1.c
> index b396458ba12..6f4d899c114 100644
> --- a/gcc/testsuite/gcc.target/powerpc/pr93453-1.c
> +++ b/gcc/testsuite/gcc.target/powerpc/pr93453-1.c
> @@ -1,5 +1,6 @@
> -/* { dg-do compile { target has_arch_ppc64 } } */
> +/* { dg-do compile } */
>  /* { dg-options "-mdejagnu-cpu=power6 -O2" } */
> +/* { dg-require-effective-target has_arch_ppc64 } */
> 
>  unsigned long load_byte_reverse (unsigned long *in)
>  {
  

On Thu, Sep 01, 2022 at 01:30:18PM +0800, HAO CHEN GUI wrote:
> --- a/gcc/testsuite/gcc.target/powerpc/pr92398.p9+.c
> +++ b/gcc/testsuite/gcc.target/powerpc/pr92398.p9+.c
> @@ -1,6 +1,10 @@
> -/* { dg-do compile { target { lp64 && has_arch_pwr9 } } } */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power9 -mvsx" } */

-mcpu=power9 already implies -mvsx.  If you would keep -mvsx, that
belongs *after* testing powerpc_vsx_ok.

> +/* { dg-require-effective-target has_arch_ppc64 } */
> +/* { dg-require-effective-target int128 } */
>  /* { dg-require-effective-target powerpc_vsx_ok } */
> -/* { dg-options "-O2 -mvsx" } */
> +/* The test case can be compiled on all platforms with compiling option
> +   -mdejagnu-cpu=power9.  */

Please don't put in comments like this: that is what the code already
*does*, after all :-)

> --- a/gcc/testsuite/gcc.target/powerpc/pr92398.p9-.c
> +++ b/gcc/testsuite/gcc.target/powerpc/pr92398.p9-.c
> @@ -1,6 +1,8 @@
> -/* { dg-do compile { target { lp64 && {! has_arch_pwr9} } } } */
> -/* { dg-require-effective-target powerpc_vsx_ok } */
>  /* { dg-options "-O2 -mvsx" } */

You cannot add -mvsx without first testing powerpc_vsx_ok (unless it is
guaranteed some other way of course; here, it isn't).

> +/* { dg-do compile { target { ! has_arch_pwr9 } } } */

Please keep dg-do first thing in the file.

> --- a/gcc/testsuite/gcc.target/powerpc/pr93453-1.c
> +++ b/gcc/testsuite/gcc.target/powerpc/pr93453-1.c
> @@ -1,5 +1,6 @@
> -/* { dg-do compile { target has_arch_ppc64 } } */
> +/* { dg-do compile } */
>  /* { dg-options "-mdejagnu-cpu=power6 -O2" } */
> +/* { dg-require-effective-target has_arch_ppc64 } */

This is fine, but it doesn't change anything, unless we have a bug.


Segher
  

Hi Kewen,

On 1/9/2022 下午 5:34, Kewen.Lin wrote:
> Thanks for the updated patch!
> 
> I just found that it seems all the three test cases suffer the empty
> TU error issue from those has_arch* effective target checks?
> 
> If yes, it looks we don't need to bother this once patch [1] gets
> landed?
> 
> Sorry, I didn't notice and ask when reviewing the previous version.
> 
> [1] https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598748.html

Yes, those 3 test cases all suffer from "empty translation unit" problem.
My patch just has an side effect which avoid "empty translation unit"
problem. But the real problem is still there.

pr92398.p9+.c has another problem. It's a compiling case and it should be
compiled on any platform when "-mdejagnu-cpu=power9" is set in dg-options
or RUNTESTFLAGS. Putting dg-options before "has_arch_pwr9" check achieves
this target.

Thanks
Gui Haochen
  

Hi Segher,
  Thanks for your review comments. I will refine it according to
your comments.

On 2/9/2022 上午 12:07, Segher Boessenkool wrote:
>> +/* { dg-do compile { target { ! has_arch_pwr9 } } } */
> Please keep dg-do first thing in the file.
Could you inform me if it's a must to put dg-do in the first line?
Here I hit a problem. "! has_arch_pwr9" can not be put into
dg-require-effective-target as it has a NOT. So I put dg-options
in the first line and make it ahead of dg-do.

> 
>> --- a/gcc/testsuite/gcc.target/powerpc/pr93453-1.c
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr93453-1.c
>> @@ -1,5 +1,6 @@
>> -/* { dg-do compile { target has_arch_ppc64 } } */
>> +/* { dg-do compile } */
>>  /* { dg-options "-mdejagnu-cpu=power6 -O2" } */
>> +/* { dg-require-effective-target has_arch_ppc64 } */
> This is fine, but it doesn't change anything, unless we have a bug.

This case suffer from "empty translation unit" problem and to be
unsupported on all platform. Put dg-options before the check avoid
the problem.

Thanks
Gui Haochen
  

on 2022/9/2 11:23, HAO CHEN GUI wrote:
> Hi Kewen,
> 
> On 1/9/2022 下午 5:34, Kewen.Lin wrote:
>> Thanks for the updated patch!
>>
>> I just found that it seems all the three test cases suffer the empty
>> TU error issue from those has_arch* effective target checks?
>>
>> If yes, it looks we don't need to bother this once patch [1] gets
>> landed?
>>
>> Sorry, I didn't notice and ask when reviewing the previous version.
>>
>> [1] https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598748.html
> 
> Yes, those 3 test cases all suffer from "empty translation unit" problem.
> My patch just has an side effect which avoid "empty translation unit"
> problem. But the real problem is still there.

OK, thanks for the information!  If so, I would prefer to leave them
alone for now, the issues should be fixed once [1] gets landed.

> 
> pr92398.p9+.c has another problem. It's a compiling case and it should be
> compiled on any platform when "-mdejagnu-cpu=power9" is set in dg-options
> or RUNTESTFLAGS. Putting dg-options before "has_arch_pwr9" check achieves
> this target.

OK, then go ahead to enhance it separately.  :)

BR,
Kewen
  

Hi!

On Fri, Sep 02, 2022 at 11:43:28AM +0800, HAO CHEN GUI wrote:
> On 2/9/2022 上午 12:07, Segher Boessenkool wrote:
> >> +/* { dg-do compile { target { ! has_arch_pwr9 } } } */
> > Please keep dg-do first thing in the file.
> Could you inform me if it's a must to put dg-do in the first line?

It is customary.  If you do differently it will be a lot harder for
people to truly understand your tests.

> Here I hit a problem. "! has_arch_pwr9" can not be put into
> dg-require-effective-target as it has a NOT.

dg-require-effective-target has a selector, maybe you can do something
with that?
  dg-require-effective-target { whatever { has_arch_pwr9 } }
or something like that?

> >> --- a/gcc/testsuite/gcc.target/powerpc/pr93453-1.c
> >> +++ b/gcc/testsuite/gcc.target/powerpc/pr93453-1.c
> >> @@ -1,5 +1,6 @@
> >> -/* { dg-do compile { target has_arch_ppc64 } } */
> >> +/* { dg-do compile } */
> >>  /* { dg-options "-mdejagnu-cpu=power6 -O2" } */
> >> +/* { dg-require-effective-target has_arch_ppc64 } */
> > This is fine, but it doesn't change anything, unless we have a bug.
> 
> This case suffer from "empty translation unit" problem and to be
> unsupported on all platform. Put dg-options before the check avoid
> the problem.

Then please fix that problem first!  It *will* come back to bite us,
multiple times per week, until it is fixed.


Segher