[PATCH] testsuite/strlenopt-81.c: Add target limitation.

[PATCH] testsuite/strlenopt-81.c: Add target limitation.

[Kito Cheng]

 - strlenopt-81.c has same limitation as strlenopt-80.c, this
   optimization only work when memcpy expand into load/store.

ChangeLog

gcc/testsuite

Kito Cheng  <kito.cheng@sifive.com>

	* gcc.dg/strlenopt-81.c: Add target limitation.
---
 gcc/testsuite/gcc.dg/strlenopt-81.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/gcc/testsuite/gcc.dg/strlenopt-81.c b/gcc/testsuite/gcc.dg/strlenopt-81.c
index 95ac29aa71f..4a0dfc4f17d 100644
--- a/gcc/testsuite/gcc.dg/strlenopt-81.c
+++ b/gcc/testsuite/gcc.dg/strlenopt-81.c
@@ -1,5 +1,9 @@
 /* PR tree-optimization/ - fold strlen relational expressions
-   { dg-do run }
+   { dg-do run { target aarch64*-*-* i?86-*-* powerpc*-*-* x86_64-*-* } }
+   The optimization is only implemented for MEM_REF stores and other
+   targets than those below may not transform the memcpy call into
+   such a store.
+
    { dg-options "-O2 -Wall -Wno-unused-local-typedefs -fdump-tree-optimized" } */
 
 typedef __SIZE_TYPE__ size_t;
-- 
2.25.0

Re: [PATCH] testsuite/strlenopt-81.c: Add target limitation.

[Martin Sebor]

On 2/13/20 8:34 PM, Kito Cheng wrote:
>   - strlenopt-81.c has same limitation as strlenopt-80.c, this
>     optimization only work when memcpy expand into load/store.

Unlike strlenopt-80.c which is a compile-time test that verifies
that the optimization successfully folds the strlen expressions,
strlenopt-81.c is a runtime test that verifies the optimization
isn't done when it might not be safe.   It shouldn't fail anywhere,
whether or not the optimization is actually done.  Are you seeing
it fail on some targets?  (That would suggest a bug.)

The test does look like it has a minor issue though: it uses
-fdump-tree-optimized but it doesn't actually scan the output for
anything.  I think it was copied from another test and not removed
by accident.  It should be removed.  (The comment at the top is
also missing the bug number that it was committed for; it should
be added.)  I can take care of this if you can confirm the above
(i.e., that there's no runtime failure on any of the targets
excluded in your patch).

Martin

> 
> ChangeLog
> 
> gcc/testsuite
> 
> Kito Cheng  <kito.cheng@sifive.com>
> 
> 	* gcc.dg/strlenopt-81.c: Add target limitation.
> ---
>   gcc/testsuite/gcc.dg/strlenopt-81.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/testsuite/gcc.dg/strlenopt-81.c b/gcc/testsuite/gcc.dg/strlenopt-81.c
> index 95ac29aa71f..4a0dfc4f17d 100644
> --- a/gcc/testsuite/gcc.dg/strlenopt-81.c
> +++ b/gcc/testsuite/gcc.dg/strlenopt-81.c
> @@ -1,5 +1,9 @@
>   /* PR tree-optimization/ - fold strlen relational expressions
> -   { dg-do run }
> +   { dg-do run { target aarch64*-*-* i?86-*-* powerpc*-*-* x86_64-*-* } }
> +   The optimization is only implemented for MEM_REF stores and other
> +   targets than those below may not transform the memcpy call into
> +   such a store.
> +
>      { dg-options "-O2 -Wall -Wno-unused-local-typedefs -fdump-tree-optimized" } */
>   
>   typedef __SIZE_TYPE__ size_t;
> 

Re: [PATCH] testsuite/strlenopt-81.c: Add target limitation.

[Kito Cheng]

> Unlike strlenopt-80.c which is a compile-time test that verifies
> that the optimization successfully folds the strlen expressions,
> strlenopt-81.c is a runtime test that verifies the optimization
> isn't done when it might not be safe.   It shouldn't fail anywhere,
> whether or not the optimization is actually done.  Are you seeing
> it fail on some targets?  (That would suggest a bug.)

Yeah, I got link fail on RISC-V, error message is:
undefined reference to `test_on_line_101_not_eliminated'

And Jim told me there is already a bug entry for this issue:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92128

It cause the __builtin_strlen not optimized/folded in test_local_cpy_4,
and the reason is blocked by __builtin_memcpy, it's same issue in
strlenopt-80.c, so I there is two way to fix this issue:

1. check result and call fail function instead of using ELIM marco.
2. Only allow part of target run this testcase like strlenopt-80.c.

This patch take the second way.

Thanks :)


On Sat, Feb 15, 2020 at 1:22 AM Martin Sebor <msebor@gmail.com> wrote:
>
> On 2/13/20 8:34 PM, Kito Cheng wrote:
> >   - strlenopt-81.c has same limitation as strlenopt-80.c, this
> >     optimization only work when memcpy expand into load/store.
>
> Unlike strlenopt-80.c which is a compile-time test that verifies
> that the optimization successfully folds the strlen expressions,
> strlenopt-81.c is a runtime test that verifies the optimization
> isn't done when it might not be safe.   It shouldn't fail anywhere,
> whether or not the optimization is actually done.  Are you seeing
> it fail on some targets?  (That would suggest a bug.)
>
> The test does look like it has a minor issue though: it uses
> -fdump-tree-optimized but it doesn't actually scan the output for
> anything.  I think it was copied from another test and not removed
> by accident.  It should be removed.  (The comment at the top is
> also missing the bug number that it was committed for; it should
> be added.)  I can take care of this if you can confirm the above
> (i.e., that there's no runtime failure on any of the targets
> excluded in your patch).
>
> Martin
>
> >
> > ChangeLog
> >
> > gcc/testsuite
> >
> > Kito Cheng  <kito.cheng@sifive.com>
> >
> >       * gcc.dg/strlenopt-81.c: Add target limitation.
> > ---
> >   gcc/testsuite/gcc.dg/strlenopt-81.c | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/gcc/testsuite/gcc.dg/strlenopt-81.c b/gcc/testsuite/gcc.dg/strlenopt-81.c
> > index 95ac29aa71f..4a0dfc4f17d 100644
> > --- a/gcc/testsuite/gcc.dg/strlenopt-81.c
> > +++ b/gcc/testsuite/gcc.dg/strlenopt-81.c
> > @@ -1,5 +1,9 @@
> >   /* PR tree-optimization/ - fold strlen relational expressions
> > -   { dg-do run }
> > +   { dg-do run { target aarch64*-*-* i?86-*-* powerpc*-*-* x86_64-*-* } }
> > +   The optimization is only implemented for MEM_REF stores and other
> > +   targets than those below may not transform the memcpy call into
> > +   such a store.
> > +
> >      { dg-options "-O2 -Wall -Wno-unused-local-typedefs -fdump-tree-optimized" } */
> >
> >   typedef __SIZE_TYPE__ size_t;
> >
>

Re: [PATCH] testsuite/strlenopt-81.c: Add target limitation.

[Jim Wilson]

On Sun, Feb 16, 2020 at 9:02 PM Kito Cheng <kito.cheng@gmail.com> wrote:
> It cause the __builtin_strlen not optimized/folded in test_local_cpy_4,
> and the reason is blocked by __builtin_memcpy, it's same issue in
> strlenopt-80.c, so I there is two way to fix this issue:

Another possible solution is to use
   { dg-require-effective-target non_strict_align }
as is done in strlenopt-72.c

If you want the testcase to work, adding __attribute__ ((aligned(4)))
to the global b and test_local_cpy_4 local a works.  That allows the
RISC-V port to convert the memcpy into an aligned load/store.  The
RISC-V port only needs the attribute added to the local variable a,
because we align char array global variables, but other targets might
not, so to be safe I think you need the alignment attribute added to
both.  However, this is only OK if you weren't trying to test
unaligned accesses here, and it isn't obvious if you were trying to do
that or not.

Jim

Re: [PATCH] testsuite/strlenopt-81.c: Add target limitation.

[Martin Sebor]

On 2/18/20 5:35 PM, Jim Wilson wrote:
> On Sun, Feb 16, 2020 at 9:02 PM Kito Cheng <kito.cheng@gmail.com> wrote:
>> It cause the __builtin_strlen not optimized/folded in test_local_cpy_4,
>> and the reason is blocked by __builtin_memcpy, it's same issue in
>> strlenopt-80.c, so I there is two way to fix this issue:
> 
> Another possible solution is to use
>     { dg-require-effective-target non_strict_align }
> as is done in strlenopt-72.c
> 
> If you want the testcase to work, adding __attribute__ ((aligned(4)))
> to the global b and test_local_cpy_4 local a works.  That allows the
> RISC-V port to convert the memcpy into an aligned load/store.  The
> RISC-V port only needs the attribute added to the local variable a,
> because we align char array global variables, but other targets might
> not, so to be safe I think you need the alignment attribute added to
> both.  However, this is only OK if you weren't trying to test
> unaligned accesses here, and it isn't obvious if you were trying to do
> that or not.

I wasn't thinking of unaligned accesses.  I like the suggestion of
adding the attribute.  Let me do some testing with it on the other
targets mentioned in the bug and take care of it today.

Thanks
Martin