On 08/08/2017 03:18 PM, Richard Biener wrote:
> On Tue, Aug 8, 2017 at 1:47 PM, Martin Liška <[email protected]> wrote:
>> On 11/09/2016 11:22 AM, Richard Biener wrote:
>>> On Fri, Nov 4, 2016 at 2:33 PM, Martin Liška <[email protected]> wrote:
>>>> On 11/03/2016 02:00 PM, Jan Hubicka wrote:
>>>>>> 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;
>>>>
>>>> Done.
>>>>
>>>>>>
>>>>>> 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.
>>>>
>>>> It's meaningful as it scans that there's no __builtin_memchr in optimized
>>>> dump.
>>>> I'm adding new tests that does the opposite test.
>>>>
>>>>>>
>>>>>>> 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.
>>>>
>>>> Yep, improved.
>>>>
>>>>>>
>>>>>>> +/* Return TRUE when CTOR can be converted to a string constant. */
>>>>>>
>>>>>> "if", not "when".
>>>>
>>>> Done.
>>>>
>>>>>>
>>>>>>> + 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
>>>>
>>>>
>>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>>
>>> I'm curious about the
>>>
>>> @@ -1495,7 +1495,8 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq *seq_p)
>>> {
>>> if (!TREE_STATIC (decl))
>>> {
>>> - DECL_INITIAL (decl) = NULL_TREE;
>>> + if (!TREE_READONLY (decl) || TREE_CODE (init) != STRING_CST)
>>> + DECL_INITIAL (decl) = NULL_TREE;
>>> init = build2 (INIT_EXPR, void_type_node, decl, init);
>>> gimplify_and_add (init, seq_p);
>>> ggc_free (init);
>>>
>>> change. Why keep DECL_INITIAL if you build a INIT_EXPR anyway?
>>>
>>> @@ -4438,6 +4439,19 @@ gimplify_init_constructor (tree *expr_p,
>>> gimple_seq *pre_p, gimple_seq *post_p,
>>> break;
>>> }
>>>
>>> + /* Replace a ctor with a string constant with possible. */
>>> + if (TREE_READONLY (object)
>>> + && VAR_P (object))
>>> + {
>>> + tree string_ctor = convert_ctor_to_string_cst (ctor);
>>> + if (string_ctor)
>>> + {
>>> + TREE_OPERAND (*expr_p, 1) = string_ctor;
>>> + DECL_INITIAL (object) = string_ctor;
>>> + break;
>>> + }
>>> + }
>>> +
>>> /* Fetch information about the constructor to direct later
>>> processing.
>>> We might want to make static versions of it in various cases, and
>>> can only do so if it known to be a valid constant initializer.
>>> */
>>>
>>> hmm, so both these hunks will end up keeping a DECL_INITIAL
>>> for non-static local consts? Usually we end up with
>>>
>>> main ()
>>> {
>>> const char local[5];
>>>
>>> <bb 2>:
>>> local = "abcd";
>>>
>>> here. So you keep DECL_INITIAL for folding?
>>>
>>> I believe we should create CONST_DECLs for the above (and make
>>> CONST_DECLs first-class
>>> symtab citizens), thus avoid runtime stack initialization for the
>>> above and instead emit
>>> "abcd" to the constant pool?
>>
>> Hi Richi.
>>
>> I've just returned to this in order to resolve long pending unfinished stuff.
>> I have couple of questions:
>>
>> a) what should create a CONST_DECL, a FE or gimplifier when it identifies
>> that
>> it can be converted to CONST_DECL?
>
> The gimplifier (prototype patches of mine did it that way when trying
> to get rid of
> &STRING_CST).
>
>> b) Why do we need to put such local variables to symtab?
>
> We don't need to, but we eventually should?
>
>> c) Do we have a target that uses CONST_DECL for such (or similar) purpose?
>
> Not sure. gimplify_init_constructor ends up with .LC0* on for example aarch64
> for initializers of arrays for example.
>
> Attached old &STRING_CST -> &CONST_DECL gimplification patch (no
> longer applies).
Can you please Richi send me the patch in unified format so that I can apply the
rejected hunks. Looks one just needs to put it into gimple-expr.c.
Thanks,
Martin
>
> Richard.
>
>> Thanks,
>> Martin
>>
>>>
>>> + /* Skip constructors with a hole. */
>>> + if (CONSTRUCTOR_NELTS (ctor) != ctor_length)
>>> + return false;
>>>
>>> not sure if that's maybe too clever ;)
>>>
>>> + FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (ctor), idx, key, value)
>>> + {
>>> + if (key == NULL_TREE
>>> + || !tree_fits_uhwi_p (key)
>>> + || !tree_fits_uhwi_p (value))
>>> + return false;
>>>
>>> I think key == NULL is very well valid (you are not using it...). I'd
>>> instead do
>>>
>>> if (key && compare_tree_int (key, idx) != 0)
>>> return false;
>>>
>>> for the hole check. value should always fit uhwi given the earlier
>>> element type check.
>>>
>>> +tree
>>> +convert_ctor_to_string_cst (tree ctor)
>>> +{
>>> + if (!can_convert_ctor_to_string_cst (ctor))
>>> + return NULL_TREE;
>>> +
>>> + unsigned HOST_WIDE_INT idx;
>>> + tree value;
>>> + vec<constructor_elt, va_gc> *elts = CONSTRUCTOR_ELTS (ctor);
>>> + char *str = XNEWVEC (char, elts->length ());
>>> +
>>> + FOR_EACH_CONSTRUCTOR_VALUE (elts, idx, value)
>>> + str[idx] = (char)tree_to_uhwi (value);
>>> +
>>> + tree init = build_string_literal (elts->length (),
>>> + ggc_alloc_string (str, elts->length ()),
>>> + false);
>>> + free (str);
>>>
>>> why alloc str on the heap, copy it to gc and the copy it again?
>>> That is, you can pass 'str' to build_string_literal directly I think.
>>>
>>> In fact as you are refactoring build_string_literal please
>>> refactor build_string into a build_string_raw taking just 'len'
>>> (thus leaving out the actual string initialization apart from
>>> 'appending' '\0') and then avoid the heap allocation.
>>>
>>> Refactor build_string_literal to take a tree STRING_CST
>>> and build_string_literal_addr to build it's address.
>>>
>>> Thanks,
>>> Richard.
>>>
>>>
>>>
>>>
>>>> Martin
>>>>
>>
