* [PATCH] strub: use opt_for_fn during ipa @ 2023-12-14 19:54 Alexandre Oliva 2023-12-15 7:08 ` Richard Biener 0 siblings, 1 reply; 4+ messages in thread From: Alexandre Oliva @ 2023-12-14 19:54 UTC (permalink / raw) To: gcc-patches; +Cc: Richard Biener Instead of global optimization levels and flags, check per-function ones. Regstrapped on x86_64-linux-gnu, also testing on sparc-solaris2.11.3. Ok to install? (sorry, Richi, I dropped the ball and failed to fix this before the monster commit) for gcc/ChangeLog * ipa-strub.cc (gsi_insert_finally_seq_after_call): Likewise. (pass_ipa_strub::adjust_at_calls_call): Likewise. --- gcc/ipa-strub.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/gcc/ipa-strub.cc b/gcc/ipa-strub.cc index 943bb60996fc1..32e2869cf7840 100644 --- a/gcc/ipa-strub.cc +++ b/gcc/ipa-strub.cc @@ -2132,7 +2132,7 @@ gsi_insert_finally_seq_after_call (gimple_stmt_iterator gsi, gimple_seq seq) || (call && gimple_call_nothrow_p (call)) || (eh_lp <= 0 && (TREE_NOTHROW (cfun->decl) - || !flag_exceptions))); + || !opt_for_fn (cfun->decl, flag_exceptions)))); if (noreturn_p && nothrow_p) return; @@ -2470,9 +2470,11 @@ pass_ipa_strub::adjust_at_calls_call (cgraph_edge *e, int named_args, /* If we're already within a strub context, pass on the incoming watermark pointer, and omit the enter and leave calls around the modified call, as an optimization, or as a means to satisfy a tail-call requirement. */ - tree swmp = ((optimize_size || optimize > 2 + tree swmp = ((opt_for_fn (e->caller->decl, optimize_size) + || opt_for_fn (e->caller->decl, optimize) > 2 || gimple_call_must_tail_p (ocall) - || (optimize == 2 && gimple_call_tail_p (ocall))) + || (opt_for_fn (e->caller->decl, optimize) == 2 + && gimple_call_tail_p (ocall))) ? strub_watermark_parm (e->caller->decl) : NULL_TREE); bool omit_own_watermark = swmp; -- Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ Free Software Activist GNU Toolchain Engineer More tolerance and less prejudice are key for inclusion and diversity Excluding neuro-others for not behaving ""normal"" is *not* inclusive ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] strub: use opt_for_fn during ipa 2023-12-14 19:54 [PATCH] strub: use opt_for_fn during ipa Alexandre Oliva @ 2023-12-15 7:08 ` Richard Biener 2023-12-19 3:54 ` Alexandre Oliva 0 siblings, 1 reply; 4+ messages in thread From: Richard Biener @ 2023-12-15 7:08 UTC (permalink / raw) To: Alexandre Oliva; +Cc: gcc-patches On Thu, 14 Dec 2023, Alexandre Oliva wrote: > > Instead of global optimization levels and flags, check per-function > ones. > > Regstrapped on x86_64-linux-gnu, also testing on sparc-solaris2.11.3. > Ok to install? You have to be generally careful when working within IPA with function bodies without push/pop_cfun around that, several APIs have variants with struct function sepcified, using the wrong one will get you a NULL cfun which _some_ of them handle gracefully and "wrong", all EH stuff is amongst this for example. I see you replace flag_exceptions with opt_for_fn (cfun->decl, flag_exceptions), given that's 'cfun' this replacement is a no-op given 'cfun' would be NULL in IPA context unless you pushed a function. Looking at the 2nd hunk and the caller it seems the transform is a no-op for indrect_calls but not callees, thus that hunk is OK. I'll note that push/pop_cfun can be quite expensive so it's worth avoiding it when possible (we do allow quite some IL manipulation from "outside" of the 'cfun', you just have to be careful with APIs) Thanks, Richard. > (sorry, Richi, I dropped the ball and failed to fix this before the > monster commit) > > > for gcc/ChangeLog > > * ipa-strub.cc (gsi_insert_finally_seq_after_call): Likewise. > (pass_ipa_strub::adjust_at_calls_call): Likewise. > --- > gcc/ipa-strub.cc | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/gcc/ipa-strub.cc b/gcc/ipa-strub.cc > index 943bb60996fc1..32e2869cf7840 100644 > --- a/gcc/ipa-strub.cc > +++ b/gcc/ipa-strub.cc > @@ -2132,7 +2132,7 @@ gsi_insert_finally_seq_after_call (gimple_stmt_iterator gsi, gimple_seq seq) > || (call && gimple_call_nothrow_p (call)) > || (eh_lp <= 0 > && (TREE_NOTHROW (cfun->decl) > - || !flag_exceptions))); > + || !opt_for_fn (cfun->decl, flag_exceptions)))); > > if (noreturn_p && nothrow_p) > return; > @@ -2470,9 +2470,11 @@ pass_ipa_strub::adjust_at_calls_call (cgraph_edge *e, int named_args, > /* If we're already within a strub context, pass on the incoming watermark > pointer, and omit the enter and leave calls around the modified call, as an > optimization, or as a means to satisfy a tail-call requirement. */ > - tree swmp = ((optimize_size || optimize > 2 > + tree swmp = ((opt_for_fn (e->caller->decl, optimize_size) > + || opt_for_fn (e->caller->decl, optimize) > 2 > || gimple_call_must_tail_p (ocall) > - || (optimize == 2 && gimple_call_tail_p (ocall))) > + || (opt_for_fn (e->caller->decl, optimize) == 2 > + && gimple_call_tail_p (ocall))) > ? strub_watermark_parm (e->caller->decl) > : NULL_TREE); > bool omit_own_watermark = swmp; > > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg) ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] strub: use opt_for_fn during ipa 2023-12-15 7:08 ` Richard Biener @ 2023-12-19 3:54 ` Alexandre Oliva 2023-12-19 8:20 ` Richard Biener 0 siblings, 1 reply; 4+ messages in thread From: Alexandre Oliva @ 2023-12-19 3:54 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches On Dec 15, 2023, Richard Biener <rguenther@suse.de> wrote: > You have to be generally careful when working within IPA > with function bodies without push/pop_cfun around that, several APIs > have variants with struct function sepcified, using the wrong one > will get you a NULL cfun which _some_ of them handle gracefully and > "wrong", all EH stuff is amongst this for example. *nod*, I recall running into that, and finding some APIs that required push/pop_cfun, so since I was implementing strub so that it could be plugged into an existing compiler, I didn't give much thought to introducing alternate APIs that could. IIRC I first hit something about EH, and then I had to put in push/pop_cfun. That was very early on, so after that I may have used implicit-cfun APIs without getting ICEs. I suppose now that strub is in pursuing push/pop_cfun avoidance could be a nice cleanup. > I see you replace flag_exceptions with opt_for_fn (cfun->decl, > flag_exceptions), given that's 'cfun' this replacement is a no-op > given 'cfun' would be NULL in IPA context unless you pushed a function. > Looking at the 2nd hunk and the caller it seems the transform is > a no-op for indrect_calls but not callees, thus that hunk is OK. Yeah, I figured that was the reason behind your recommendation, but I guess adding explicit uses of cfun (rather than passing a function around) doesn't really make things much better, except inasmuchas it enables a future de-cfun-ification of strub passes to be a little more mechanical. -- Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ Free Software Activist GNU Toolchain Engineer More tolerance and less prejudice are key for inclusion and diversity Excluding neuro-others for not behaving ""normal"" is *not* inclusive ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] strub: use opt_for_fn during ipa 2023-12-19 3:54 ` Alexandre Oliva @ 2023-12-19 8:20 ` Richard Biener 0 siblings, 0 replies; 4+ messages in thread From: Richard Biener @ 2023-12-19 8:20 UTC (permalink / raw) To: Alexandre Oliva; +Cc: gcc-patches On Tue, 19 Dec 2023, Alexandre Oliva wrote: > On Dec 15, 2023, Richard Biener <rguenther@suse.de> wrote: > > > You have to be generally careful when working within IPA > > with function bodies without push/pop_cfun around that, several APIs > > have variants with struct function sepcified, using the wrong one > > will get you a NULL cfun which _some_ of them handle gracefully and > > "wrong", all EH stuff is amongst this for example. > > *nod*, I recall running into that, and finding some APIs that required > push/pop_cfun, so since I was implementing strub so that it could be > plugged into an existing compiler, I didn't give much thought to > introducing alternate APIs that could. IIRC I first hit something about > EH, and then I had to put in push/pop_cfun. That was very early on, so > after that I may have used implicit-cfun APIs without getting ICEs. I > suppose now that strub is in pursuing push/pop_cfun avoidance could be a > nice cleanup. Yeah, adding API variants with explicit struct function is considered OK when that allows to avoid push/pop_cfun. When playing with stmts the first thing you'll hit is update_stmt (though it's core worker, update_stmt_operands has the arg already). Sometimes the cfun dependence isn't really obvious ... > > I see you replace flag_exceptions with opt_for_fn (cfun->decl, > > flag_exceptions), given that's 'cfun' this replacement is a no-op > > given 'cfun' would be NULL in IPA context unless you pushed a function. > > > Looking at the 2nd hunk and the caller it seems the transform is > > a no-op for indrect_calls but not callees, thus that hunk is OK. > > Yeah, I figured that was the reason behind your recommendation, but I > guess adding explicit uses of cfun (rather than passing a function > around) doesn't really make things much better, except inasmuchas it > enables a future de-cfun-ification of strub passes to be a little more > mechanical. Yep, we've gone through some of GCCs APIs where there was two variants, one with and one without explicit struct function arg and eliminating the first in favor of explicit mentioning of 'cfun' to show where we depend on that. That was in the context of eventually threading parts of GCC which of course doesn't play well with this kind of global state. Richard. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-12-19 8:21 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-12-14 19:54 [PATCH] strub: use opt_for_fn during ipa Alexandre Oliva 2023-12-15 7:08 ` Richard Biener 2023-12-19 3:54 ` Alexandre Oliva 2023-12-19 8:20 ` Richard Biener
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).