On Mon, 20 Jan 2014, Jakub Jelinek wrote:
> On Mon, Jan 20, 2014 at 11:54:15AM +0100, Richard Biener wrote:
> > Ok, the following simpler patch also fixes the issue and causes
> > no testsuite fallout - we restrict the folding in builtins.c to
> > the case with known src strlen.
>
> But then we simply never optimize this anymore.
> Or regress when you e.g. in your testcase replace
> __builtin___strcat_chk with __builtin_strcat and remove last argument
> (while if there is no inline, it is still optimized).
>
> tree-ssa-strlen.c apparently both doesn't this case (unknown first strlen,
> known second strlen or movstr pattern, will only transform that if the
> length of the resulting string is needed afterwards), and isn't run
> for -Os or -O1 anyway.
Well, I'm not sure under which circumstances this should be an
unconditional win anyway (I expect the strcat library fn to be
optimized well enough, and only if you can avoid the strlen on the
dest in the end it will be profitable)
> For other builtins, this case is usually handled in gimple-fold.c
> (gimple_fold_builtin), but BUILT_IN_STRCAT isn't handled there.
> So perhaps it should and add one argument to fold_builtin_strcat
> if we know the length, but don't know where the argument points to.
>
> And/or handle src being SSA_NAME with GIMPLE_PHI def stmt, by
> calling c_strlen on each of the phi arguments individually (i.e. handle
> it similarly to the COND_EXPR case) and verifying it is the same.
>
> But I guess if we optimize it again, your testcase would crash again, right?
Right. We can apply the optimization at RTL expansion time though,
or handle the folding completely in gimple-fold with not needing to
dispatch to the gimplifier.
Richard.
> > 2014-01-17 Richard Biener <rguent...@suse.de>
> >
> > PR middle-end/59860
> > * builtins.c (fold_builtin_strcat): Remove case better handled
> > by tree-ssa-strlen.c.
> >
> > * gcc.dg/pr59860.c: New testcase.
> >
> > Index: gcc/testsuite/gcc.dg/pr59860.c
> > ===================================================================
> > *** gcc/testsuite/gcc.dg/pr59860.c (revision 0)
> > --- gcc/testsuite/gcc.dg/pr59860.c (working copy)
> > ***************
> > *** 0 ****
> > --- 1,15 ----
> > + /* { dg-do compile } */
> > + /* { dg-options "-O" } */
> > +
> > + extern __inline __attribute__ ((__always_inline__)) __attribute__
> > ((__gnu_inline__)) __attribute__ ((__artificial__)) char * __attribute__
> > ((__nothrow__ , __leaf__))
> > + strcat (char *__restrict __dest, const char *__restrict __src)
> > + {
> > + return __builtin___strcat_chk (__dest, __src, __builtin_object_size
> > (__dest, 2 > 1));
> > + }
> > + static char raw_decode;
> > + void foo (char **argv, char *outfilename)
> > + {
> > + if (**argv == 'r')
> > + raw_decode = 1;
> > + strcat (outfilename, raw_decode ? ".raw" : ".wav");
> > + }
> > Index: gcc/builtins.c
> > ===================================================================
> > *** gcc/builtins.c (revision 206772)
> > --- gcc/builtins.c (working copy)
> > *************** fold_builtin_strcat (location_t loc ATTR
> > *** 11759,11775 ****
> > if (!strlen_fn || !strcpy_fn)
> > return NULL_TREE;
> >
> > ! /* If we don't have a movstr we don't want to emit an strcpy
> > ! call. We have to do that if the length of the source string
> > ! isn't computable (in that case we can use memcpy probably
> > ! later expanding to a sequence of mov instructions). If we
> > ! have movstr instructions we can emit strcpy calls. */
> > ! if (!HAVE_movstr)
> > ! {
> > ! tree len = c_strlen (src, 1);
> > ! if (! len || TREE_SIDE_EFFECTS (len))
> > ! return NULL_TREE;
> > ! }
> >
> > /* Stabilize the argument list. */
> > dst = builtin_save_expr (dst);
> > --- 11759,11769 ----
> > if (!strlen_fn || !strcpy_fn)
> > return NULL_TREE;
> >
> > ! /* If the length of the source string isn't computable don't
> > ! split strcat into strlen and strcpy. */
> > ! tree len = c_strlen (src, 1);
> > ! if (! len || TREE_SIDE_EFFECTS (len))
> > ! return NULL_TREE;
> >
> > /* Stabilize the argument list. */
> > dst = builtin_save_expr (dst);
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer
