| Message ID | 7802386.q05VISrV2B@polaris |
|---|---|
| State | New |
| Headers | show |
On Fri, Sep 13, 2013 at 4:29 PM, Eric Botcazou <ebotcazou@adacore.com> wrote: > Hi, > > in Ada parameters can be passed by reference: in this case, the address of the > argument is directly passed to the callee, which dereferences it to access the > argument; now Ada also enables -fexceptions -fnon-call-exceptions, which means > that any pointer dereference is seen as the potential source of an exception, > which can quickly block the optimizer. That's why we put TREE_THIS_NOTRAP on > the dereferences associated with a parameter. > > This works fine as long as the function isn't inlined, because it may happen > that the argument is itself the dereference of a pointer, properly guarded by > a null check on the pointer. When the function is inlined, the dereference in > the caller is replaced with that of the callee, which is TREE_THIS_NOTRAP and > thus can be moved ahead of the null pointer check, for example by LIM. > > The patch ensures that this cannot happen by clearing TREE_THIS_NOTRAP in the > inliner. I think that this affects only the Ada compiler in practice. > > Tested on x86_64-suse-linux, OK for the mainline? I've looked at the C++ testcase int foo (int &x) { try { return x; } catch (...) { return 0; } } which exhibits exactly the behavior you quote - return x is considered throwing an exception. The C++ FE doesn't arrange for TREE_THIS_NOTRAP to be set here (maybe due to this issue you quote?). Other than that the patch looks reasonable (I suppose you need is_parameter_of only because as we recursively handle the trees PARM_DECLs from the destination could already have leaked into the tree we recurse into?) Thanks, Richard. > > 2013-09-13 Eric Botcazou <ebotcazou@adacore.com> > > * tree-inline.h (struct copy_body_data): Add transform_parameter. > * tree-inline.c (is_parameter_of): New predicate. > (remap_gimple_op_r): Do not propagate TREE_THIS_NOTRAP on MEM_REF if > a parameter has been remapped. > (copy_tree_body_r): Likewise on INDIRECT_REF and MEM_REF. > (optimize_inline_calls): Initialize transform_parameter. > (unsave_expr_now): Likewise. > (copy_gimple_seq_and_replace_locals): Likewise. > (tree_function_versioning): Likewise. > (maybe_inline_call_in_expr): Likewise. > > > 2013-09-13 Eric Botcazou <ebotcazou@adacore.com> > > * gnat.dg/opt27.adb: New test. > * gnat.dg/opt27_pkg.ad[sb]: New helper. > > > -- > Eric Botcazou
On 09/16/2013 05:28 AM, Richard Biener wrote: > which exhibits exactly the behavior you quote - return x is considered throwing > an exception. The C++ FE doesn't arrange for TREE_THIS_NOTRAP to be > set here (maybe due to this issue you quote?). I haven't been aware of TREE_THIS_NOTRAP, but we could certainly start setting it. Jason
> I've looked at the C++ testcase > > int foo (int &x) > { > try { > return x; > } > catch (...) > { > return 0; > } > } > > which exhibits exactly the behavior you quote - return x is considered > throwing an exception. The C++ FE doesn't arrange for TREE_THIS_NOTRAP to > be set here (maybe due to this issue you quote?). I presume that you compiled with -fnon-call-exceptions? Otherwise, I don't see how something that isn't a call can throw an exception in C++, it should be seen at most as possibly trapping, which is less blocking. > Other than that the patch looks reasonable (I suppose you need > is_parameter_of only because as we recursively handle the trees > PARM_DECLs from the destination could already have leaked into > the tree we recurse into?) Do you mean that the test on DECL_CONTEXT is superfluous? Possibly indeed, but with nested functions you can have PARM_DECLs of different origins in a given function body, although this may be irrelevant for tree-inline.c.
On Tue, Sep 17, 2013 at 9:03 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> I've looked at the C++ testcase >> >> int foo (int &x) >> { >> try { >> return x; >> } >> catch (...) >> { >> return 0; >> } >> } >> >> which exhibits exactly the behavior you quote - return x is considered >> throwing an exception. The C++ FE doesn't arrange for TREE_THIS_NOTRAP to >> be set here (maybe due to this issue you quote?). > > I presume that you compiled with -fnon-call-exceptions? Otherwise, I don't > see how something that isn't a call can throw an exception in C++, it should > be seen at most as possibly trapping, which is less blocking. Yes, with -fnon-call-exceptions. >> Other than that the patch looks reasonable (I suppose you need >> is_parameter_of only because as we recursively handle the trees >> PARM_DECLs from the destination could already have leaked into >> the tree we recurse into?) > > Do you mean that the test on DECL_CONTEXT is superfluous? Possibly indeed, > but with nested functions you can have PARM_DECLs of different origins in a > given function body, although this may be irrelevant for tree-inline.c. Yeah, I thought testing for a PARM_DECL should be sufficient? For nested functions references to outer parms should have been lowered via the static chain at the point tree-inline.c sees them. So, if you agree that the DECL_CONTEXT test is superfluous the patch is ok with the is_parameter_of function removed. Thanks, Richard. > -- > Eric Botcazou
> Yeah, I thought testing for a PARM_DECL should be sufficient? For > nested functions references to outer parms should have been lowered via the > static chain at the point tree-inline.c sees them. OK for the latter point, but are you sure for the former? My understanding is that we're already in SSA form, so parameters can be represented by SSA_NAMEs without defining statements.
On Fri, Sep 13, 2013 at 04:29:48PM +0200, Eric Botcazou wrote: > @@ -4748,6 +4774,8 @@ copy_gimple_seq_and_replace_locals (gimp > id.transform_call_graph_edges = CB_CGE_DUPLICATE; > id.transform_new_cfg = false; > id.transform_return_to_modify = false; > + id.transform_parameter = false; > + id.transform_parameter = false; > id.transform_lang_insert_block = NULL; > > /* Walk the tree once to find local labels. */ Why are you storing the same thing twice? Jakub
On Tue, Sep 17, 2013 at 10:42 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> Yeah, I thought testing for a PARM_DECL should be sufficient? For >> nested functions references to outer parms should have been lowered via the >> static chain at the point tree-inline.c sees them. > > OK for the latter point, but are you sure for the former? My understanding is > that we're already in SSA form, so parameters can be represented by SSA_NAMEs > without defining statements. That's true... so you can only simplify is_parameter_of by dropping the context check. Richard. > -- > Eric Botcazou
> That's true... so you can only simplify is_parameter_of by dropping > the context check. OK, thanks, installed with this modification and the fix for the oversight spotted by Jakub, after retesting on x86-64/Linux.
Index: tree-inline.c =================================================================== --- tree-inline.c (revision 202431) +++ tree-inline.c (working copy) @@ -751,6 +751,21 @@ copy_gimple_bind (gimple stmt, copy_body return new_bind; } +/* Return true if DECL is a parameter or a SSA_NAME for a parameter of + function FNDECL. */ + +static bool +is_parameter_of (tree decl, tree fndecl) +{ + if (TREE_CODE (decl) == SSA_NAME) + { + decl = SSA_NAME_VAR (decl); + if (!decl) + return false; + } + + return (TREE_CODE (decl) == PARM_DECL && DECL_CONTEXT (decl) == fndecl); +} /* Remap the GIMPLE operand pointed to by *TP. DATA is really a 'struct walk_stmt_info *'. DATA->INFO is a 'copy_body_data *'. @@ -840,20 +855,25 @@ remap_gimple_op_r (tree *tp, int *walk_s if (TREE_CODE (*tp) == MEM_REF) { - tree ptr = TREE_OPERAND (*tp, 0); - tree type = remap_type (TREE_TYPE (*tp), id); - tree old = *tp; - /* We need to re-canonicalize MEM_REFs from inline substitutions that can happen when a pointer argument is an ADDR_EXPR. Recurse here manually to allow that. */ + tree ptr = TREE_OPERAND (*tp, 0); + tree type = remap_type (TREE_TYPE (*tp), id); + tree old = *tp; walk_tree (&ptr, remap_gimple_op_r, data, NULL); - *tp = fold_build2 (MEM_REF, type, - ptr, TREE_OPERAND (*tp, 1)); - TREE_THIS_NOTRAP (*tp) = TREE_THIS_NOTRAP (old); + *tp = fold_build2 (MEM_REF, type, ptr, TREE_OPERAND (*tp, 1)); TREE_THIS_VOLATILE (*tp) = TREE_THIS_VOLATILE (old); TREE_SIDE_EFFECTS (*tp) = TREE_SIDE_EFFECTS (old); TREE_NO_WARNING (*tp) = TREE_NO_WARNING (old); + /* We cannot propagate the TREE_THIS_NOTRAP flag if we have + remapped a parameter as the property might be valid only + for the parameter itself. */ + if (TREE_THIS_NOTRAP (old) + && (!is_parameter_of (TREE_OPERAND (old, 0), id->src_fn) + || (!id->transform_parameter + && is_parameter_of (ptr, id->dst_fn)))) + TREE_THIS_NOTRAP (*tp) = 1; *walk_subtrees = 0; return NULL; } @@ -1043,45 +1063,45 @@ copy_tree_body_r (tree *tp, int *walk_su /* Get rid of *& from inline substitutions that can happen when a pointer argument is an ADDR_EXPR. */ tree decl = TREE_OPERAND (*tp, 0); - tree *n; - - n = (tree *) pointer_map_contains (id->decl_map, decl); + tree *n = (tree *) pointer_map_contains (id->decl_map, decl); if (n) { - tree new_tree; - tree old; /* If we happen to get an ADDR_EXPR in n->value, strip it manually here as we'll eventually get ADDR_EXPRs which lie about their types pointed to. In this case build_fold_indirect_ref wouldn't strip the INDIRECT_REF, but we absolutely rely on that. As fold_indirect_ref does other useful transformations, try that first, though. */ - tree type = TREE_TYPE (TREE_TYPE (*n)); - if (id->do_not_unshare) - new_tree = *n; - else - new_tree = unshare_expr (*n); - old = *tp; - *tp = gimple_fold_indirect_ref (new_tree); + tree type = TREE_TYPE (*tp); + tree ptr = id->do_not_unshare ? *n : unshare_expr (*n); + tree old = *tp; + *tp = gimple_fold_indirect_ref (ptr); if (! *tp) { - if (TREE_CODE (new_tree) == ADDR_EXPR) + if (TREE_CODE (ptr) == ADDR_EXPR) { - *tp = fold_indirect_ref_1 (EXPR_LOCATION (new_tree), - type, new_tree); + *tp + = fold_indirect_ref_1 (EXPR_LOCATION (ptr), type, ptr); /* ??? We should either assert here or build a VIEW_CONVERT_EXPR instead of blindly leaking incompatible types to our IL. */ if (! *tp) - *tp = TREE_OPERAND (new_tree, 0); + *tp = TREE_OPERAND (ptr, 0); } else { - *tp = build1 (INDIRECT_REF, type, new_tree); + *tp = build1 (INDIRECT_REF, type, ptr); TREE_THIS_VOLATILE (*tp) = TREE_THIS_VOLATILE (old); TREE_SIDE_EFFECTS (*tp) = TREE_SIDE_EFFECTS (old); TREE_READONLY (*tp) = TREE_READONLY (old); - TREE_THIS_NOTRAP (*tp) = TREE_THIS_NOTRAP (old); + /* We cannot propagate the TREE_THIS_NOTRAP flag if we + have remapped a parameter as the property might be + valid only for the parameter itself. */ + if (TREE_THIS_NOTRAP (old) + && (!is_parameter_of (decl, id->src_fn) + || (!id->transform_parameter + && is_parameter_of (ptr, id->dst_fn)))) + TREE_THIS_NOTRAP (*tp) = 1; } } *walk_subtrees = 0; @@ -1090,20 +1110,25 @@ copy_tree_body_r (tree *tp, int *walk_su } else if (TREE_CODE (*tp) == MEM_REF) { - tree ptr = TREE_OPERAND (*tp, 0); - tree type = remap_type (TREE_TYPE (*tp), id); - tree old = *tp; - /* We need to re-canonicalize MEM_REFs from inline substitutions that can happen when a pointer argument is an ADDR_EXPR. Recurse here manually to allow that. */ + tree ptr = TREE_OPERAND (*tp, 0); + tree type = remap_type (TREE_TYPE (*tp), id); + tree old = *tp; walk_tree (&ptr, copy_tree_body_r, data, NULL); - *tp = fold_build2 (MEM_REF, type, - ptr, TREE_OPERAND (*tp, 1)); - TREE_THIS_NOTRAP (*tp) = TREE_THIS_NOTRAP (old); + *tp = fold_build2 (MEM_REF, type, ptr, TREE_OPERAND (*tp, 1)); TREE_THIS_VOLATILE (*tp) = TREE_THIS_VOLATILE (old); TREE_SIDE_EFFECTS (*tp) = TREE_SIDE_EFFECTS (old); TREE_NO_WARNING (*tp) = TREE_NO_WARNING (old); + /* We cannot propagate the TREE_THIS_NOTRAP flag if we have + remapped a parameter as the property might be valid only + for the parameter itself. */ + if (TREE_THIS_NOTRAP (old) + && (!is_parameter_of (TREE_OPERAND (old, 0), id->src_fn) + || (!id->transform_parameter + && is_parameter_of (ptr, id->dst_fn)))) + TREE_THIS_NOTRAP (*tp) = 1; *walk_subtrees = 0; return NULL; } @@ -4443,6 +4468,7 @@ optimize_inline_calls (tree fn) id.transform_call_graph_edges = CB_CGE_DUPLICATE; id.transform_new_cfg = false; id.transform_return_to_modify = true; + id.transform_parameter = true; id.transform_lang_insert_block = NULL; id.statements_to_fold = pointer_set_create (); @@ -4748,6 +4774,8 @@ copy_gimple_seq_and_replace_locals (gimp id.transform_call_graph_edges = CB_CGE_DUPLICATE; id.transform_new_cfg = false; id.transform_return_to_modify = false; + id.transform_parameter = false; + id.transform_parameter = false; id.transform_lang_insert_block = NULL; /* Walk the tree once to find local labels. */ @@ -5207,6 +5235,7 @@ tree_function_versioning (tree old_decl, = update_clones ? CB_CGE_MOVE_CLONES : CB_CGE_MOVE; id.transform_new_cfg = true; id.transform_return_to_modify = false; + id.transform_parameter = false; id.transform_lang_insert_block = NULL; old_entry_block = ENTRY_BLOCK_PTR_FOR_FUNCTION @@ -5431,6 +5460,7 @@ maybe_inline_call_in_expr (tree exp) id.transform_call_graph_edges = CB_CGE_DUPLICATE; id.transform_new_cfg = false; id.transform_return_to_modify = true; + id.transform_parameter = true; id.transform_lang_insert_block = NULL; /* Make sure not to unshare trees behind the front-end's back Index: tree-inline.h =================================================================== --- tree-inline.h (revision 202431) +++ tree-inline.h (working copy) @@ -97,6 +97,10 @@ typedef struct copy_body_data by manipulating the CFG rather than a statement. */ bool transform_return_to_modify; + /* True if the parameters of the source function are transformed. + Only true for inlining. */ + bool transform_parameter; + /* True if this statement will need to be regimplified. */ bool regimplify;