On Thu, May 16, 2013 at 04:28:19PM +0200, Jakub Jelinek wrote: > On Thu, May 16, 2013 at 04:18:27PM +0200, Jakub Jelinek wrote: > > As q could point to p, if we didn't do what your patch does on the p[0] = > > 'X'; > > store, then we'd need to invalidate the recorded length of the q string. > > Similarly if there is p[0] = '\0' or p[0] = var. > > Ah, another thing while we are at it. > For p[0] = '\0'; case when p[0] is known to be '\0' already, we > remove it if: > /* When storing '\0', the store can be removed > if we know it has been stored in the current function. */ > if (!stmt_could_throw_p (stmt) && si->writable) > (and in that case don't invalidate anything either). But the above > condition is false, we set si->writable (correct) and si->dont_invalidate, > while we could do instead of that the same what you do for non-zero store to > non-zero location, i.e. gsi_next (gsi); return false;. > Perhaps a testcase for that is: > size_t > bar (char *p, char *r) > { > size_t len1 = strlen (r); > char *q = strchr (p, '\0'); > *q = '\0'; > return len1 - strlen (r); // This strlen should be optimized into len1. > } > > strlen (q) should be known to be zero at that point, but si->writable should > be false, we don't know if p doesn't point say into .rodata, and > stmt_could_throw_p probably should return true too.
Nice, thanks! In the patch below I hopefully addressed all the issues; I also set si->writable to false, but we can't return true in the stmt_could_throw_p case. So, how does it look now? Regtested/bootstrapped on x86_64-linux. 2013-05-16 Marek Polacek <pola...@redhat.com> * tree-ssa-strlen.c (handle_char_store): Don't invalidate cached length when doing non-zero store of storing '\0' to '\0', don't mark string as writable. * gcc.dg/strlenopt-25.c: New test. * gcc.dg/strlenopt-26.c: Likewise. --- gcc/tree-ssa-strlen.c.mp 2013-05-15 14:11:20.079707492 +0200 +++ gcc/tree-ssa-strlen.c 2013-05-16 17:57:33.963150006 +0200 @@ -1693,8 +1693,10 @@ handle_char_store (gimple_stmt_iterator } else { - si->writable = true; - si->dont_invalidate = true; + /* The string might be e.g. in the .rodata section. */ + si->writable = false; + gsi_next (gsi); + return false; } } else @@ -1717,6 +1719,33 @@ handle_char_store (gimple_stmt_iterator si->endptr = ssaname; si->dont_invalidate = true; } + /* If si->length is non-zero constant, we aren't overwriting '\0', + and if we aren't storing '\0', we know that the length of the + string and any other zero terminated string in memory remains + the same. In that case we move to the next gimple statement and + return to signal the caller that it shouldn't invalidate anything. + + This is benefical for cases like: + + char p[20]; + void foo (char *q) + { + strcpy (p, "foobar"); + size_t len = strlen (p); // This can be optimized into 6 + size_t len2 = strlen (q); // This has to be computed + p[0] = 'X'; + size_t len3 = strlen (p); // This can be optimized into 6 + size_t len4 = strlen (q); // This can be optimized into len2 + bar (len, len2, len3, len4); + } + */ + else if (si != NULL && si->length != NULL_TREE + && TREE_CODE (si->length) == INTEGER_CST + && integer_nonzerop (gimple_assign_rhs1 (stmt))) + { + gsi_next (gsi); + return false; + } } else if (idx == 0 && initializer_zerop (gimple_assign_rhs1 (stmt))) { --- gcc/testsuite/gcc.dg/strlenopt-25.c.mp 2013-05-15 17:15:18.702118637 +0200 +++ gcc/testsuite/gcc.dg/strlenopt-25.c 2013-05-15 18:26:27.881030317 +0200 @@ -0,0 +1,18 @@ +/* { dg-do run } */ +/* { dg-options "-O2 -fdump-tree-strlen" } */ + +#include "strlenopt.h" + +int +main () +{ + char p[] = "foobar"; + int len, len2; + len = strlen (p); + p[0] = 'O'; + len2 = strlen (p); + return len - len2; +} + +/* { dg-final { scan-tree-dump-times "strlen \\(" 0 "strlen" } } */ +/* { dg-final { cleanup-tree-dump "strlen" } } */ --- gcc/testsuite/gcc.dg/strlenopt-26.c.mp 2013-05-16 17:33:00.302060413 +0200 +++ gcc/testsuite/gcc.dg/strlenopt-26.c 2013-05-16 18:30:51.906342948 +0200 @@ -0,0 +1,25 @@ +/* { dg-do run } */ +/* { dg-options "-O2 -fdump-tree-strlen" } */ + +#include "strlenopt.h" + +__attribute__((noinline, noclone)) size_t +fn1 (char *p, const char *r) +{ + size_t len1 = __builtin_strlen (r); + char *q = __builtin_strchr (p, '\0'); + *q = '\0'; + return len1 - __builtin_strlen (r); // This strlen should be optimized into len1. +} + +int +main (void) +{ + char p[] = "foobar"; + const char *volatile q = "xyzzy"; + fn1 (p, q); + return 0; +} + +/* { dg-final { scan-tree-dump-times "strlen \\(" 1 "strlen" } } */ +/* { dg-final { cleanup-tree-dump "strlen" } } */ Marek