On Tue, Nov 1, 2016 at 4:20 PM, Jakub Jelinek <[email protected]> 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?
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
