Re: [PATCH] strlenopt improvements
On Sat, Nov 05, 2011 at 11:44:27AM +0100, Eric Botcazou wrote:
> Thanks for the tip. Tested on Solaris 8 and Linux, applied on the mainline.
Thanks.
> @@ -32,10 +31,9 @@ main ()
>return 0;
> }
>
> -/* { dg-final { scan-tree-dump-times "strlen \\(" 0 "strlen" } } */
> +/* { dg-final { scan-tree-dump-times "strlen \\(" 3 "strlen" } } */
> /* { dg-final { scan-tree-dump-times "memcpy \\(" 1 "strlen" } } */
> -/* { dg-final { scan-tree-dump-times "strcpy \\(" 0 "strlen" } } */
> +/* { dg-final { scan-tree-dump-times "strcpy \\(" 1 "strlen" } } */
> /* { dg-final { scan-tree-dump-times "strcat \\(" 0 "strlen" } } */
> /* { dg-final { scan-tree-dump-times "strchr \\(" 1 "strlen" } } */
> -/* { dg-final { scan-tree-dump-times "stpcpy \\(" 1 "strlen" } } */
This line perhaps could have been kept, just with s/1/0/ to also test
that when stpcpy prototype isn't provided we don't emit any stpcpy
calls in code that didn't originally have any of them.
Jakub
Re: [PATCH] strlenopt improvements
> The man page is outdated, stpcpy is a standard POSIX2008 function.
Sorry for being so 20th century-ish. :-)
> Anyway, in the other gcc.dg/strlenopt-* testcases for USE_GNU I was using
> the convention that the name ended with g (i.e. strlenopt-22g.c) and
> the test would start with:
> /* This test needs runtime that provides stpcpy function. */
> /* { dg-do run { target *-*-linux* } } */
> instead of just
> /* { dg-do run } */
Thanks for the tip. Tested on Solaris 8 and Linux, applied on the mainline.
2011-11-05 Eric Botcazou
* gcc.dg/strlenopt-22g.c: New wrapper around...
* gcc.dg/strlenopt-22.c: ...this. Do not define USE_GNU and adjust.
--
Eric Botcazou
Index: gcc.dg/strlenopt-22g.c
===
--- gcc.dg/strlenopt-22g.c (revision 0)
+++ gcc.dg/strlenopt-22g.c (revision 0)
@@ -0,0 +1,14 @@
+/* This test needs runtime that provides stpcpy function. */
+/* { dg-do run { target *-*-linux* } } */
+/* { dg-options "-O2 -fdump-tree-strlen" } */
+
+#define USE_GNU
+#include "strlenopt-22.c"
+
+/* { dg-final { scan-tree-dump-times "strlen \\(" 0 "strlen" } } */
+/* { dg-final { scan-tree-dump-times "memcpy \\(" 1 "strlen" } } */
+/* { dg-final { scan-tree-dump-times "strcpy \\(" 0 "strlen" } } */
+/* { dg-final { scan-tree-dump-times "strcat \\(" 0 "strlen" } } */
+/* { dg-final { scan-tree-dump-times "strchr \\(" 1 "strlen" } } */
+/* { dg-final { scan-tree-dump-times "stpcpy \\(" 1 "strlen" } } */
+/* { dg-final { cleanup-tree-dump "strlen" } } */
Index: gcc.dg/strlenopt-22.c
===
--- gcc.dg/strlenopt-22.c (revision 181007)
+++ gcc.dg/strlenopt-22.c (working copy)
@@ -1,7 +1,6 @@
/* { dg-do run } */
/* { dg-options "-O2 -fdump-tree-strlen" } */
-#define USE_GNU
#include "strlenopt.h"
__attribute__((noinline, noclone)) size_t
@@ -32,10 +31,9 @@ main ()
return 0;
}
-/* { dg-final { scan-tree-dump-times "strlen \\(" 0 "strlen" } } */
+/* { dg-final { scan-tree-dump-times "strlen \\(" 3 "strlen" } } */
/* { dg-final { scan-tree-dump-times "memcpy \\(" 1 "strlen" } } */
-/* { dg-final { scan-tree-dump-times "strcpy \\(" 0 "strlen" } } */
+/* { dg-final { scan-tree-dump-times "strcpy \\(" 1 "strlen" } } */
/* { dg-final { scan-tree-dump-times "strcat \\(" 0 "strlen" } } */
/* { dg-final { scan-tree-dump-times "strchr \\(" 1 "strlen" } } */
-/* { dg-final { scan-tree-dump-times "stpcpy \\(" 1 "strlen" } } */
/* { dg-final { cleanup-tree-dump "strlen" } } */
Re: [PATCH] strlenopt improvements
On Wed, Nov 02, 2011 at 02:55:54PM +0100, Rainer Orth wrote:
> Jakub Jelinek writes:
>
> > Anyway, in the other gcc.dg/strlenopt-* testcases for USE_GNU I was using
> > the convention that the name ended with g (i.e. strlenopt-22g.c) and
> > the test would start with:
> > /* This test needs runtime that provides stpcpy function. */
> > /* { dg-do run { target *-*-linux* } } */
> > instead of just
> > /* { dg-do run } */
>
> This isn't right either: e.g. Solaris 11 does have stpcpy, as do systems
> with a non-Linux kernel, but using *glibc. If there are more than one or
> two instances of this, this needs an effective-target keyword, along the
> lines of target-supports.exp (check_mkfifo_available).
Feel free to change it, it isn't too important to me, all it would buy
us is additional testing of generic optimizations on a tiny bit more
targets.
Some strlenopt-*f.c tests also need __*_chk functions, either just those,
or those and stpcpy.
Jakub
Re: [PATCH] strlenopt improvements
Jakub Jelinek writes:
> Anyway, in the other gcc.dg/strlenopt-* testcases for USE_GNU I was using
> the convention that the name ended with g (i.e. strlenopt-22g.c) and
> the test would start with:
> /* This test needs runtime that provides stpcpy function. */
> /* { dg-do run { target *-*-linux* } } */
> instead of just
> /* { dg-do run } */
This isn't right either: e.g. Solaris 11 does have stpcpy, as do systems
with a non-Linux kernel, but using *glibc. If there are more than one or
two instances of this, this needs an effective-target keyword, along the
lines of target-supports.exp (check_mkfifo_available).
Rainer
--
-
Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [PATCH] strlenopt improvements
On Wed, Nov 02, 2011 at 01:41:30PM +0100, Eric Botcazou wrote:
> > 2011-10-24 Andreas Krebbel
> >
> > * gcc.dg/strlenopt-22.c: New testcase.
>
> This doesn't link if you don't have stpcpy in the libc, e.g. on Solaris.
>
> Here's an excerpt from the Linux man pages:
>
> CONFORMING TO
>This function is not part of the C or POSIX.1 standards, and is not
>customary on Unix systems, but is not a GNU invention either. Perhaps
>it comes from MS-DOS.
The man page is outdated, stpcpy is a standard POSIX2008 function.
Anyway, in the other gcc.dg/strlenopt-* testcases for USE_GNU I was using
the convention that the name ended with g (i.e. strlenopt-22g.c) and
the test would start with:
/* This test needs runtime that provides stpcpy function. */
/* { dg-do run { target *-*-linux* } } */
instead of just
/* { dg-do run } */
Jakub
Re: [PATCH] strlenopt improvements
> 2011-10-24 Andreas Krebbel > > * gcc.dg/strlenopt-22.c: New testcase. This doesn't link if you don't have stpcpy in the libc, e.g. on Solaris. Here's an excerpt from the Linux man pages: CONFORMING TO This function is not part of the C or POSIX.1 standards, and is not customary on Unix systems, but is not a GNU invention either. Perhaps it comes from MS-DOS. -- Eric Botcazou
Re: [PATCH] strlenopt improvements
On Mon, Oct 24, 2011 at 10:04:45PM +0200, Andreas Krebbel wrote: > > Can you please explain this stmt_set_p stuff? dont_invalidate should be > > only set on strinfos that will be seen by the immediately following > > maybe_invalidate call (at the end of handle_builtin_strcpy caller - > > strlen_optimize_stmt). If you set it on which unshare_strinfo is called, > > if there is no unsharing, it is obviously fine, but if there is unsharing, > > then dont_invalidate will be set on some strinfo that won't be seen by the > > next may_invalidate. It might trigger in some other basic block and might > > cause wrong code. > > It's not needed. I've retested it with the following loop instead. No > regressions on s390x and x86_64. Ok to apply? Yes, thanks. Jakub
Re: [PATCH] strlenopt improvements
> Can you please explain this stmt_set_p stuff? dont_invalidate should be
> only set on strinfos that will be seen by the immediately following
> maybe_invalidate call (at the end of handle_builtin_strcpy caller -
> strlen_optimize_stmt). If you set it on which unshare_strinfo is called,
> if there is no unsharing, it is obviously fine, but if there is unsharing,
> then dont_invalidate will be set on some strinfo that won't be seen by the
> next may_invalidate. It might trigger in some other basic block and might
> cause wrong code.
It's not needed. I've retested it with the following loop instead. No
regressions on s390x and x86_64. Ok to apply?
+
+ if (dsi->prev != 0 && (chainsi = verify_related_strinfos (dsi)) != NULL)
+ {
+ for (; chainsi && chainsi != dsi; chainsi = get_strinfo
(chainsi->next))
+ {
+ /* When setting a stmt for delayed length computation
+prevent all strinfos through dsi from being
+invalidated. */
+ chainsi = unshare_strinfo (chainsi);
+ chainsi->stmt = stmt;
+ chainsi->length = NULL_TREE;
+ chainsi->endptr = NULL_TREE;
+ chainsi->dont_invalidate = true;
+ }
+ }
Bye,
-Andreas-
Re: [PATCH] strlenopt improvements
On Mon, Oct 24, 2011 at 07:15:14PM +0200, Andreas Krebbel wrote:
> + if (dsi->prev != 0 && (chainsi = verify_related_strinfos (dsi)) !=
> NULL)
> + {
> + bool stmt_set_p = false;
> +
> + for (; chainsi && chainsi != dsi; chainsi = get_strinfo
> (chainsi->next))
> + {
> + /* When setting a stmt for delayed length computation
> + prevent all strinfos through dsi from being
> + invalidated. */
> + if (stmt_set_p)
> + chainsi->dont_invalidate = true;
> +
> + chainsi = unshare_strinfo (chainsi);
> + chainsi->stmt = stmt;
> + chainsi->length = NULL_TREE;
> + chainsi->endptr = NULL_TREE;
> + chainsi->dont_invalidate = true;
> + stmt_set_p = true;
> + }
> + }
Can you please explain this stmt_set_p stuff? dont_invalidate should be
only set on strinfos that will be seen by the immediately following
maybe_invalidate call (at the end of handle_builtin_strcpy caller -
strlen_optimize_stmt). If you set it on which unshare_strinfo is called,
if there is no unsharing, it is obviously fine, but if there is unsharing,
then dont_invalidate will be set on some strinfo that won't be seen by the
next may_invalidate. It might trigger in some other basic block and might
cause wrong code.
Otherwise it looks good.
Jakub
[PATCH] strlenopt improvements
Hi,
the attached patch fixes all the strlenopt failures on s390x (without
nuking the strcat folding).
The one case I couldn't get working so far is the second strlen in:
__attribute__((noinline, noclone)) size_t
bar (char *p, char *q)
{
char *r;
size_t l1, l2;
r = strchr (p, '\0');
strcpy (r, q);
l1 = strlen (p);
strcpy (r, "567");
l2 = strlen (p);
return l1 + l2;
}
Perhaps this could be fixed by putting a stmt_addend value into the
strinfo structs.
Bootstrapped on x86_64 and s390x. No regressions.
Ok for mainline?
Bye,
-Andreas-
2011-10-24 Andreas Krebbel
* tree-ssa-strlen.c (get_string_length): Change assertion to STPCPY.
(zero_length_string): Change assertion to accept strinfo without
length but with stmt instead.
Set the endptr pointer also if starting a new chain.
(adjust_related_strinfos): Ignore strinfos marked for delayed
length computation.
(handle_builtin_strcpy): Mark earlier strinfo elements also for
delayed length computation.
2011-10-24 Andreas Krebbel
* gcc.dg/strlenopt-22.c: New testcase.
* gcc.dg/strlenopt-4.c: Change scan value for s390(x).
Index: gcc/tree-ssa-strlen.c
===
*** gcc/tree-ssa-strlen.c.orig
--- gcc/tree-ssa-strlen.c
*** get_string_length (strinfo si)
*** 397,403
callee = gimple_call_fndecl (stmt);
gcc_assert (callee && DECL_BUILT_IN_CLASS (callee) == BUILT_IN_NORMAL);
lhs = gimple_call_lhs (stmt);
! gcc_assert (builtin_decl_implicit_p (BUILT_IN_STRCPY));
/* unshare_strinfo is intentionally not called here. The (delayed)
transformation of strcpy or strcat into stpcpy is done at the place
of the former strcpy/strcat call and so can affect all the strinfos
--- 397,403
callee = gimple_call_fndecl (stmt);
gcc_assert (callee && DECL_BUILT_IN_CLASS (callee) == BUILT_IN_NORMAL);
lhs = gimple_call_lhs (stmt);
! gcc_assert (builtin_decl_implicit_p (BUILT_IN_STPCPY));
/* unshare_strinfo is intentionally not called here. The (delayed)
transformation of strcpy or strcat into stpcpy is done at the place
of the former strcpy/strcat call and so can affect all the strinfos
*** zero_length_string (tree ptr, strinfo ch
*** 588,600
|| si->prev != chainsi->idx)
break;
}
! gcc_assert (chainsi->length);
if (chainsi->endptr == NULL_TREE)
{
chainsi = unshare_strinfo (chainsi);
chainsi->endptr = ptr;
}
! if (integer_zerop (chainsi->length))
{
if (chainsi->next)
{
--- 588,600
|| si->prev != chainsi->idx)
break;
}
! gcc_assert (chainsi->length || chainsi->stmt);
if (chainsi->endptr == NULL_TREE)
{
chainsi = unshare_strinfo (chainsi);
chainsi->endptr = ptr;
}
! if (chainsi->length && integer_zerop (chainsi->length))
{
if (chainsi->next)
{
*** zero_length_string (tree ptr, strinfo ch
*** 626,631
--- 626,633
if (chainsi->first == 0)
chainsi->first = chainsi->idx;
chainsi->next = idx;
+ if (chainsi->endptr == NULL_TREE)
+ chainsi->endptr = ptr;
si->prev = chainsi->idx;
si->first = chainsi->first;
si->writable = chainsi->writable;
*** adjust_related_strinfos (location_t loc,
*** 654,664
tree tem;
si = unshare_strinfo (si);
! gcc_assert (si->length);
! tem = fold_convert_loc (loc, TREE_TYPE (si->length), adj);
! si->length = fold_build2_loc (loc, PLUS_EXPR,
! TREE_TYPE (si->length), si->length,
! tem);
si->endptr = NULL_TREE;
si->dont_invalidate = true;
}
--- 656,674
tree tem;
si = unshare_strinfo (si);
! if (si->length)
! {
! tem = fold_convert_loc (loc, TREE_TYPE (si->length), adj);
! si->length = fold_build2_loc (loc, PLUS_EXPR,
! TREE_TYPE (si->length), si->length,
! tem);
! }
! else if (si->stmt != NULL)
! /* Delayed length computation is unaffected. */
! ;
! else
! gcc_unreachable ();
!
si->endptr = NULL_TREE;
si->dont_invalidate = true;
}
*** handle_builtin_strcpy (enum built_in_fun
*** 1117,1126
--- 1127,1162
if (dsi->length == NULL_TREE)
{
+ strinfo chainsi;
+
/* If string length of src is unknown, use delayed l
