| Message ID | CAD_=9DScPttApycW+odoTfyaot4hbcqpLLO69N0BPTyOFW36Uw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 1 Dec 2011, Diego Novillo wrote: > On Thu, Dec 1, 2011 at 07:08, Richard Guenther <rguenther@suse.de> wrote: > > Sure, but then you can still have the issue of an inconsistency. > > Not if we make the edge attribute secondary to the statement > attribute. Given that can_inline_edge_p() is the *only* tester for > this attribute, what I was thinking was to change can_inline_edge_p() > to: > > diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c > index 3dadf8d..e3c6b3c 100644 > --- a/gcc/ipa-inline.c > +++ b/gcc/ipa-inline.c > @@ -246,6 +246,14 @@ can_inline_edge_p (struct cgraph_edge *e, bool report) > struct function *caller_cfun = DECL_STRUCT_FUNCTION (e->caller->decl); > struct function *callee_cfun > = callee ? DECL_STRUCT_FUNCTION (callee->decl) : NULL; > + bool call_stmt_cannot_inline_p; > + > + /* If E has a call statement in it, use the inline attribute from > + the statement, otherwise use the inline attribute in E. Edges > + will not have statements when working in WPA mode. */ > + call_stmt_cannot_inline_p = (e->call_stmt) > + ? gimple_call_cannot_inline_p (e->call_stmt) > + : e->call_stmt_cannot_inline_p; > > if (!caller_cfun && e->caller->clone_of) > caller_cfun = DECL_STRUCT_FUNCTION (e->caller->clone_of->decl); > @@ -270,7 +278,7 @@ can_inline_edge_p (struct cgraph_edge *e, bool report) > e->inline_failed = CIF_OVERWRITABLE; > return false; > } > - else if (e->call_stmt_cannot_inline_p) > + else if (call_stmt_cannot_inline_p) > { > e->inline_failed = CIF_MISMATCHED_ARGUMENTS; > inlinable = false; > @@ -343,14 +351,6 @@ can_inline_edge_p (struct cgraph_edge *e, bool report) > } > } > > - /* Be sure that the cannot_inline_p flag is up to date. */ > - gcc_checking_assert (!e->call_stmt > - || (gimple_call_cannot_inline_p (e->call_stmt) > - == e->call_stmt_cannot_inline_p) > - /* In -flto-partition=none mode we really keep > things out of > - sync because call_stmt_cannot_inline_p is > set at cgraph > - merging when function bodies are not there yet. */ > - || (in_lto_p && !gimple_call_cannot_inline_p > (e->call_stmt))); > if (!inlinable && report) > report_inline_failed_reason (e); > return inlinable; > > > > > Thus, would you then remove the remaining asserts? > > The asserts disappear because we have weakened the meaning of the edge > attribute. It is only usable when there is no statement on it. The > question now is, how do we know that the attribute is not lying? This > only happens in WPA mode, so it would then become an issue of > pessimization, not correctness. The above looks ok to me, but I don't want the gimple_call_set_cannot_inline change (if it is in the tree - I have not yet recovered from three weeks of vacation). The edge attribute is "recomputed" when necessary. > > I believe in the end the proper fix is to _not_ throw away > > cgraph edges all the time, but keep them up-to-date and thus > > make the stmt flag not necessary. > > Make it a pure cgraph attribute? Sure, anything that gets rid of the > dual attribute is the way to go. There are not very many invocations > to the gimple attribute, but I don't know how big a change that is. The issue with that change would be to preserve the cgraph edges. Though when we create them we always have the call stmt available and thus could re-compute that flag. Honza? > > Which pass did the folding of the stmt but did not adjust the > > edge flag? > > The new call to gimple_call_set_cannot_inline added by this patch: Sure, but what _pass_ changed the call stmt and called fold_stmt on it? The patch merely changes the flag during folding. Richard.
On Thu, Dec 1, 2011 at 09:04, Richard Guenther <rguenther@suse.de> wrote: > The above looks ok to me, but I don't want the > gimple_call_set_cannot_inline change (if it is in the tree - I have > not yet recovered from three weeks of vacation). The edge attribute > is "recomputed" when necessary. The original patch is no longer necessary given this change of semantics we are discussing. For the original semantics, it was needed because every change to the statement state must be reflected in the edge. If we make the edge attribute invisible in the presence of a call statement, then it doesn't matter if we update it or not. The only issue I have now is that if we allow the edge attribute to go stale, when we save it to a bytecode file, and use it during WPA, we will be using the stale value. We could update the edge attribute every time can_inline_edge_p is called, but I'm not sure I like that. >> > Which pass did the folding of the stmt but did not adjust the >> > edge flag? >> >> The new call to gimple_call_set_cannot_inline added by this patch: > > Sure, but what _pass_ changed the call stmt and called fold_stmt > on it? The patch merely changes the flag during folding. Ah. #0 gimple_call_set_cannot_inline (s=0x7ffff0b88ab0, inlinable_p=true) at pph/gcc/gimple.c:5570 #1 0x00000000008c6d0b in gimple_fold_call (inplace=<optimized out>, gsi=<optimized out>) at pph/gcc/gimple-fold.c:1121 #2 fold_stmt_1 (gsi=0x7fffffffd580, inplace=false) at pph/gcc/gimple-fold.c:1198 #3 0x0000000000a7a775 in fold_marked_statements (first=6, statements=0x16c9090) at pph/gcc/tree-inline.c:4174 #4 0x0000000000a88a0b in tree_function_versioning (old_decl=<optimized out>, new_decl=0x7ffff4be1000, tree_map=<optimized out>, update_clones=224, args_to_skip=<optimized out>, blocks_to_copy=0x1a28d30, new_entry=0x7ffff04c7958) at pph/gcc/tree-inline.c:5259 #5 0x0000000000782c27 in cgraph_function_versioning ( old_version_node=<optimized out>, redirect_callers=0x0, tree_map=0x0, args_to_skip=0x1a25b50, bbs_to_copy=0x1a28d30, new_entry_block=0x7ffff04c7958, clone_name=0x10227f4 "part") at pph/gcc/cgraphunit.c:2383 #6 0x0000000000ee2452 in split_function (split_point=<optimized out>) at pph/gcc/ipa-split.c:1102 #7 execute_split_functions () at pph/gcc/ipa-split.c:1412 #8 0x0000000000990f15 in execute_one_pass (pass=0x1413060) Line numbers are relative to the PPH branch, which is trunk as of a couple of weeks ago. I *think* the bug only triggers with -m32 -march=pentium3, but I am not sure (need to rebuild the .ii file). Diego.
On Thu, 1 Dec 2011, Diego Novillo wrote: > On Thu, Dec 1, 2011 at 09:04, Richard Guenther <rguenther@suse.de> wrote: > > > The above looks ok to me, but I don't want the > > gimple_call_set_cannot_inline change (if it is in the tree - I have > > not yet recovered from three weeks of vacation). The edge attribute > > is "recomputed" when necessary. > > The original patch is no longer necessary given this change of > semantics we are discussing. For the original semantics, it was > needed because every change to the statement state must be reflected > in the edge. If we make the edge attribute invisible in the presence > of a call statement, then it doesn't matter if we update it or not. > > The only issue I have now is that if we allow the edge attribute to go > stale, when we save it to a bytecode file, and use it during WPA, we > will be using the stale value. We still update it in a few selected places. > We could update the edge attribute every time can_inline_edge_p is > called, but I'm not sure I like that. Me neither. > >> > Which pass did the folding of the stmt but did not adjust the > >> > edge flag? > >> > >> The new call to gimple_call_set_cannot_inline added by this patch: > > > > Sure, but what _pass_ changed the call stmt and called fold_stmt > > on it? The patch merely changes the flag during folding. > > Ah. > > #0 gimple_call_set_cannot_inline (s=0x7ffff0b88ab0, inlinable_p=true) > at pph/gcc/gimple.c:5570 > #1 0x00000000008c6d0b in gimple_fold_call (inplace=<optimized out>, > gsi=<optimized out>) > at pph/gcc/gimple-fold.c:1121 > #2 fold_stmt_1 (gsi=0x7fffffffd580, inplace=false) > at pph/gcc/gimple-fold.c:1198 > #3 0x0000000000a7a775 in fold_marked_statements (first=6, > statements=0x16c9090) > at pph/gcc/tree-inline.c:4174 > #4 0x0000000000a88a0b in tree_function_versioning (old_decl=<optimized out>, > new_decl=0x7ffff4be1000, tree_map=<optimized out>, update_clones=224, > args_to_skip=<optimized out>, blocks_to_copy=0x1a28d30, > new_entry=0x7ffff04c7958) > at pph/gcc/tree-inline.c:5259 > #5 0x0000000000782c27 in cgraph_function_versioning ( > old_version_node=<optimized out>, redirect_callers=0x0, tree_map=0x0, > args_to_skip=0x1a25b50, bbs_to_copy=0x1a28d30, > new_entry_block=0x7ffff04c7958, clone_name=0x10227f4 "part") > at pph/gcc/cgraphunit.c:2383 > #6 0x0000000000ee2452 in split_function (split_point=<optimized out>) > at pph/gcc/ipa-split.c:1102 > #7 execute_split_functions () > at pph/gcc/ipa-split.c:1412 > #8 0x0000000000990f15 in execute_one_pass (pass=0x1413060) > > Line numbers are relative to the PPH branch, which is trunk as of a > couple of weeks ago. I *think* the bug only triggers with -m32 > -march=pentium3, but I am not sure (need to rebuild the .ii file). ISTR updating the function cloning path, so you might be simply on a too old trunk version. Do you have the 2011-11-06 change? And the 2011-11-09 change? But your proposed change looks ok anyway with reverting the original patch. Thanks, Richard.
diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c index 3dadf8d..e3c6b3c 100644 --- a/gcc/ipa-inline.c +++ b/gcc/ipa-inline.c @@ -246,6 +246,14 @@ can_inline_edge_p (struct cgraph_edge *e, bool report) struct function *caller_cfun = DECL_STRUCT_FUNCTION (e->caller->decl); struct function *callee_cfun = callee ? DECL_STRUCT_FUNCTION (callee->decl) : NULL; + bool call_stmt_cannot_inline_p; + + /* If E has a call statement in it, use the inline attribute from + the statement, otherwise use the inline attribute in E. Edges + will not have statements when working in WPA mode. */ + call_stmt_cannot_inline_p = (e->call_stmt) + ? gimple_call_cannot_inline_p (e->call_stmt) + : e->call_stmt_cannot_inline_p; if (!caller_cfun && e->caller->clone_of) caller_cfun = DECL_STRUCT_FUNCTION (e->caller->clone_of->decl); @@ -270,7 +278,7 @@ can_inline_edge_p (struct cgraph_edge *e, bool report) e->inline_failed = CIF_OVERWRITABLE; return false; } - else if (e->call_stmt_cannot_inline_p) + else if (call_stmt_cannot_inline_p) { e->inline_failed = CIF_MISMATCHED_ARGUMENTS; inlinable = false; @@ -343,14 +351,6 @@ can_inline_edge_p (struct cgraph_edge *e, bool report) } } - /* Be sure that the cannot_inline_p flag is up to date. */ - gcc_checking_assert (!e->call_stmt - || (gimple_call_cannot_inline_p (e->call_stmt) - == e->call_stmt_cannot_inline_p) - /* In -flto-partition=none mode we really keep things out of - sync because call_stmt_cannot_inline_p is set at cgraph - merging when function bodies are not there yet. */ - || (in_lto_p && !gimple_call_cannot_inline_p (e->call_stmt)));