> On 11/03/2016 01:12 PM, Martin Liška wrote:
> >+ tree init = DECL_INITIAL (decl);
> >+ if (init
> >+ && TREE_READONLY (decl)
> >+ && can_convert_ctor_to_string_cst (init))
> >+ DECL_INITIAL (decl) = build_string_cst_from_ctor (init);
>
> I'd merge these two new functions since they're only ever called
> together. We'd then have something like
>
> if (init && TREE_READONLY (decl))
> init = convert_ctor_to_string_cst (init);
> if (init)
> DECL_INITIAL (decl) = init;
>
> I'll defer to Jan on whether finalize_decl seems like a good place
> to do this.
I think finalize_decl may be bit too early because frontends may expects the
ctors to be in a way they produced them. We only want to convert those arrays
seen by middle-end so I would move the logic to varpool_node::analyze
Otherwise the patch seems fine to me.
Honza
>
> >diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple.c
> >b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple.c
> >index 283bd1c..b2d1fd5 100644
> >--- a/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple.c
> >+++ b/gcc/testsuite/gcc.dg/tree-ssa/builtins-folding-gimple.c
> >@@ -4,12 +4,15 @@
> > char *buffer1;
> > char *buffer2;
> >
> >+const char global[] = {'a', 'b', 'c', 'd', '\0'};
> >+
> > #define SIZE 1000
> >
> > int
> > main (void)
> > {
> > const char* const foo1 = "hello world";
> >+ const char local[] = "abcd";
> >
> > buffer1 = __builtin_malloc (SIZE);
> > __builtin_strcpy (buffer1, foo1);
> >@@ -45,6 +48,10 @@ main (void)
> > __builtin_abort ();
> > if (__builtin_memchr (foo1, null, 12) != foo1 + 11)
> > __builtin_abort ();
> >+ if (__builtin_memchr (global, null, 5) == 0)
> >+ __builtin_abort ();
> >+ if (__builtin_memchr (local, null, 5) == 0)
> >+ __builtin_abort ();
>
> How is that a meaningful test? This seems to work even with an
> unpatched gcc. I'd have expected something that shows a benefit for
> doing this conversion, and maybe also a test that shows it isn't
> done in cases where it's not allowed.
>
> > tree
> >-build_string_literal (int len, const char *str)
> >+build_string_literal (int len, const char *str, bool build_addr_expr)
>
> New arguments should be documented in the function comment.
>
> >+/* Return TRUE when CTOR can be converted to a string constant. */
>
> "if", not "when".
>
> >+ unsigned HOST_WIDE_INT elements = CONSTRUCTOR_NELTS (ctor);
> >+ FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (ctor), idx, key, value)
> >+ {
> >+ if (key == NULL_TREE
> >+ || TREE_CODE (key) != INTEGER_CST
> >+ || !tree_fits_uhwi_p (value)
> >+ || !useless_type_conversion_p (TREE_TYPE (value), char_type_node))
> >+ return false;
>
> Shouldn't all elements have the same type, or do you really have to
> call useless_type_conversion in a loop?
>
> >+ /* Allow zero character just at the end of a string. */
> >+ if (integer_zerop (value))
> >+ return idx == elements - 1;
>
> Don't you also have to explicitly check it's there?
>
>
> Bernd
