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.

        Jakub

Reply via email to