RFA (tree-inline): PATCH for C++ inheriting constructors overhaul

On Tue, Nov 1, 2016 at 4:20 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Oct 31, 2016 at 11:45:08AM -0400, Jason Merrill wrote:
>> Is the tree-inline.c patch OK for trunk?
>
>> --- a/gcc/tree-inline.c
>> +++ b/gcc/tree-inline.c
>> @@ -1241,6 +1241,28 @@ copy_tree_body_r (tree *tp, int *walk_subtrees, void *data)
>>         *walk_subtrees = 0;
>>         return NULL;
>>       }
>> +      else if (TREE_CODE (*tp) == COND_EXPR)
>> +     {
>> +       tree cond = TREE_OPERAND (*tp, 0);
>> +       walk_tree (&cond, copy_tree_body_r, data, NULL);
>> +       cond = fold (cond);
>> +       if (TREE_CODE (cond) == INTEGER_CST)
>> +         {
>> +           /* Only copy the taken branch; for a C++ base constructor clone
>> +              inherited from a virtual base, copying the other branch leads
>> +              to references to parameters that were optimized away.  */
>> +           tree branch = (integer_nonzerop (cond)
>> +                          ? TREE_OPERAND (*tp, 1)
>> +                          : TREE_OPERAND (*tp, 2));
>> +           tree type = TREE_TYPE (*tp);
>> +           if (VOID_TYPE_P (type)
>> +               || type == TREE_TYPE (branch))
>> +             {
>> +               *tp = branch;
>> +               return copy_tree_body_r (tp, walk_subtrees, data);
>> +             }
>> +         }
>> +     }
>
> This doesn't look right to me.  I believe if we walk_tree copy_tree_body_r
> any operand, we have to walk all of them and *walk_subtrees = 0;, otherwise
> we'll effectively walk and copy possibly huge condition twice (which can
> have very bad compile time / memory effects if the condition has many
> COND_EXPRs in it).
> So I think if you don't return copy_tree_body_r (tp, walk_subtrees, data);,
> you should do something like:
>         copy_tree_r (tp, walk_subtrees, NULL);
>         TREE_OPERAND (*tp, 0) = cond;
>         walk_tree (&TREE_OPERAND (*tp, 1), copy_tree_body_r, data, NULL);
>         walk_tree (&TREE_OPERAND (*tp, 2), copy_tree_body_r, data, NULL);
>         *walk_subtrees = 0;
> and then somehow arrange to continue after the
>         copy_tree_r (tp, walk_subtrees, NULL);
> a few lines later, in particular the TREE_SET_BLOCK stuff, and
> remap_type stuff (dunno if goto, or conditionalize the copy_tree_r there
> on *walk_subtrees != 0, or what).
> The case of a constant cond is likely ok, tp should already initially
> point to an operand of a newly copied tree, just with the old COND_EXPR,
> so if we replace it with a branch and recurse that should handle all the
> copying/remapping etc.

Like so?

On Tue, Nov 01, 2016 at 05:33:17PM -0400, Jason Merrill wrote:
> Like so?

LGTM, thanks.

> diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
> index de5e575..6899d2a 100644
> --- a/gcc/tree-inline.c
> +++ b/gcc/tree-inline.c
> @@ -1045,6 +1045,7 @@ copy_tree_body_r (tree *tp, int *walk_subtrees, void *data)
>    copy_body_data *id = (copy_body_data *) data;
>    tree fn = id->src_fn;
>    tree new_block;
> +  bool copied = false;
>  
>    /* Begin by recognizing trees that we'll completely rewrite for the
>       inlining context.  Our output for these trees is completely
> @@ -1241,10 +1242,40 @@ copy_tree_body_r (tree *tp, int *walk_subtrees, void *data)
>  	  *walk_subtrees = 0;
>  	  return NULL;
>  	}
> +      else if (TREE_CODE (*tp) == COND_EXPR)
> +	{
> +	  tree cond = TREE_OPERAND (*tp, 0);
> +	  walk_tree (&cond, copy_tree_body_r, data, NULL);
> +	  tree folded = fold (cond);
> +	  if (TREE_CODE (folded) == INTEGER_CST)
> +	    {
> +	      /* Only copy the taken branch; for a C++ base constructor clone
> +		 inherited from a virtual base, copying the other branch leads
> +		 to references to parameters that were optimized away.  */
> +	      tree branch = (integer_nonzerop (folded)
> +			     ? TREE_OPERAND (*tp, 1)
> +			     : TREE_OPERAND (*tp, 2));
> +	      tree type = TREE_TYPE (*tp);
> +	      if (VOID_TYPE_P (type)
> +		  || type == TREE_TYPE (branch))
> +		{
> +		  *tp = branch;
> +		  return copy_tree_body_r (tp, walk_subtrees, data);
> +		}
> +	    }
> +	  /* Avoid copying the condition twice.  */
> +	  copy_tree_r (tp, walk_subtrees, NULL);
> +	  TREE_OPERAND (*tp, 0) = cond;
> +	  walk_tree (&TREE_OPERAND (*tp, 1), copy_tree_body_r, data, NULL);
> +	  walk_tree (&TREE_OPERAND (*tp, 2), copy_tree_body_r, data, NULL);
> +	  *walk_subtrees = 0;
> +	  copied = true;
> +	}
>  
>        /* Here is the "usual case".  Copy this tree node, and then
>  	 tweak some special cases.  */
> -      copy_tree_r (tp, walk_subtrees, NULL);
> +      if (!copied)
> +	copy_tree_r (tp, walk_subtrees, NULL);
>  
>        /* If EXPR has block defined, map it to newly constructed block.
>           When inlining we want EXPRs without block appear in the block


	Jakub