Re: [patch] Introduce deep unsharing

[Richard Guenther <richard dot guenther at gmail dot com>]

On Tue, May 18, 2010 at 12:36 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Mon, May 17, 2010 at 10:42 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>>> So maybe you don't want to gimplify type sizes but only gimplify
>>> references to them inside COMPONENT_REFs and ARRAY_REFs?
>>> And those are all via SAVE_EXPRs that are shared between
>>> expressions across functions? ? So you'd need to copy the SAVE_EXPR
>>> itself before gimplifying it? ?At least I don't see how only copying
>>> its argument is going to help you - the SAVE_EXPR itself will still
>>> be shared and you modify all shared uses.
>>
>> No, SAVE_EXPRs must never be shared between functions since you cannot unshare
>> them. ?So we never generate SAVE_EXPRs at the global level, including in size
>> expressions for global types. ?The problem is regular SAVE_EXPRs generated in
>> functions and which happen to reference size expressions in their subtree.
>
> Ah, so the SAVE_EXPRs itself are not shared between functions
> but its operand is? ?So when the gimplifier makes its way down
> it modifies shared trees?
>
> Wouldn't it be simpler (or would it be possible at all) to always
> unshare the SAVE_EXPR operand in gimplify_save_expr
> if !SAVE_EXPR_RESOLVED_P? ?That would avoid the pointer-map
> and I'd say we can do so unconditionally.
>
> I'm not sure if the shared TARGET_EXPR use is constrained
> so we can play a similar trick.
>
> Well, at least I think I do understand the problem now - do I?
> I find the patch ugly because of the new langhook - it looks like
> that this either "changes" GENERIC semantics or that we
> do not properly handle what is valid GENERIC. ?Can you try
> enabling the unsharing by default and see how memory usage
> and compile time is affected for say C++?

I'm thinking of

Index: gimplify.c
===================================================================
--- gimplify.c  (revision 159476)
+++ gimplify.c  (working copy)
@@ -4658,12 +4658,14 @@ gimplify_save_expr (tree *expr_p, gimple
   /* If the SAVE_EXPR has not been resolved, then evaluate it once.  */
   if (!SAVE_EXPR_RESOLVED_P (*expr_p))
     {
+      val = unshare_expr (val);
+
       /* The operand may be a void-valued expression such as SAVE_EXPRs
         generated by the Java frontend for class initialization.  It is
         being executed only for its side-effects.  */
       if (TREE_TYPE (val) == void_type_node)
        {
-         ret = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, post_p,
+         ret = gimplify_expr (&val, pre_p, post_p,
                               is_gimple_stmt, fb_none);
          val = NULL;
        }


as you don't seem to need the TARGET_EXPR handling (the
FE didn't try to handle it before either).  The patch doesn't work
on the controlled1.ads testcase, but I don't see why.  It seems
to be an issue with unnesting.

Richard.