| Message ID | AM4PR0701MB216250333677E5E83BA62D3AE4090@AM4PR0701MB2162.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jul 21, 2016 at 11:04:48AM +0000, Bernd Edlinger wrote: > bool > +gimple_alloca_call_p (const gimple *stmt) > +{ > + tree fndecl; > + > + if (!is_gimple_call (stmt)) > + return false; > + > + fndecl = gimple_call_fndecl (stmt); > + if (fndecl && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL) > + switch (DECL_FUNCTION_CODE (fndecl)) > + { > + case BUILT_IN_ALLOCA: > + case BUILT_IN_ALLOCA_WITH_ALIGN: > + return true; > + } This should have failed bootstrap because of -Wswitch-enum. You need default: break; in. > + switch (DECL_FUNCTION_CODE (fndecl)) > + { > + case BUILT_IN_ALLOCA: > + case BUILT_IN_ALLOCA_WITH_ALIGN: > + return true; Likewise here. Jakub
On 07/21/2016 01:16 PM, Jakub Jelinek wrote: > On Thu, Jul 21, 2016 at 11:04:48AM +0000, Bernd Edlinger wrote: >> bool >> +gimple_alloca_call_p (const gimple *stmt) >> +{ >> + tree fndecl; >> + >> + if (!is_gimple_call (stmt)) >> + return false; >> + >> + fndecl = gimple_call_fndecl (stmt); >> + if (fndecl && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL) >> + switch (DECL_FUNCTION_CODE (fndecl)) >> + { >> + case BUILT_IN_ALLOCA: >> + case BUILT_IN_ALLOCA_WITH_ALIGN: >> + return true; >> + } > > This should have failed bootstrap because of -Wswitch-enum. > You need > default: > break; > in. > >> + switch (DECL_FUNCTION_CODE (fndecl)) >> + { >> + case BUILT_IN_ALLOCA: >> + case BUILT_IN_ALLOCA_WITH_ALIGN: >> + return true; > > Likewise here. > Or write it in the more natural way as an if. Bernd
On 07/21/16 13:16, Jakub Jelinek wrote: > On Thu, Jul 21, 2016 at 11:04:48AM +0000, Bernd Edlinger wrote: >> bool >> +gimple_alloca_call_p (const gimple *stmt) >> +{ >> + tree fndecl; >> + >> + if (!is_gimple_call (stmt)) >> + return false; >> + >> + fndecl = gimple_call_fndecl (stmt); >> + if (fndecl && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL) >> + switch (DECL_FUNCTION_CODE (fndecl)) >> + { >> + case BUILT_IN_ALLOCA: >> + case BUILT_IN_ALLOCA_WITH_ALIGN: >> + return true; >> + } > > This should have failed bootstrap because of -Wswitch-enum. > You need > default: > break; > in. > >> + switch (DECL_FUNCTION_CODE (fndecl)) >> + { >> + case BUILT_IN_ALLOCA: >> + case BUILT_IN_ALLOCA_WITH_ALIGN: >> + return true; > > Likewise here. > Thanks, confirmed. I added that now. Bernd. > Jakub >
On Thu, 21 Jul 2016, Bernd Edlinger wrote: > On 07/21/16 13:16, Jakub Jelinek wrote: > > On Thu, Jul 21, 2016 at 11:04:48AM +0000, Bernd Edlinger wrote: > >> bool > >> +gimple_alloca_call_p (const gimple *stmt) > >> +{ > >> + tree fndecl; > >> + > >> + if (!is_gimple_call (stmt)) > >> + return false; > >> + > >> + fndecl = gimple_call_fndecl (stmt); > >> + if (fndecl && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL) > >> + switch (DECL_FUNCTION_CODE (fndecl)) > >> + { > >> + case BUILT_IN_ALLOCA: > >> + case BUILT_IN_ALLOCA_WITH_ALIGN: > >> + return true; > >> + } > > > > This should have failed bootstrap because of -Wswitch-enum. > > You need > > default: > > break; > > in. > > > >> + switch (DECL_FUNCTION_CODE (fndecl)) > >> + { > >> + case BUILT_IN_ALLOCA: > >> + case BUILT_IN_ALLOCA_WITH_ALIGN: > >> + return true; > > > > Likewise here. > > > > Thanks, confirmed. > I added that now. Ok with that change. Thanks, Richard. > Bernd. > > > Jakub > > > >
On 07/21/16 13:25, Bernd Schmidt wrote: > > > On 07/21/2016 01:16 PM, Jakub Jelinek wrote: >> On Thu, Jul 21, 2016 at 11:04:48AM +0000, Bernd Edlinger wrote: >>> bool >>> +gimple_alloca_call_p (const gimple *stmt) >>> +{ >>> + tree fndecl; >>> + >>> + if (!is_gimple_call (stmt)) >>> + return false; >>> + >>> + fndecl = gimple_call_fndecl (stmt); >>> + if (fndecl && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL) >>> + switch (DECL_FUNCTION_CODE (fndecl)) >>> + { >>> + case BUILT_IN_ALLOCA: >>> + case BUILT_IN_ALLOCA_WITH_ALIGN: >>> + return true; >>> + } >> >> This should have failed bootstrap because of -Wswitch-enum. >> You need >> default: >> break; >> in. >> >>> + switch (DECL_FUNCTION_CODE (fndecl)) >>> + { >>> + case BUILT_IN_ALLOCA: >>> + case BUILT_IN_ALLOCA_WITH_ALIGN: >>> + return true; >> >> Likewise here. >> > Or write it in the more natural way as an if. > I'm open for that suggestion. Then I should probably also rewrite the switch statement in special_function_p as an if. Thanks Bernd. > > Bernd >
On Thu, 21 Jul 2016, Bernd Edlinger wrote: > On 07/21/16 13:25, Bernd Schmidt wrote: > > > > > > On 07/21/2016 01:16 PM, Jakub Jelinek wrote: > >> On Thu, Jul 21, 2016 at 11:04:48AM +0000, Bernd Edlinger wrote: > >>> bool > >>> +gimple_alloca_call_p (const gimple *stmt) > >>> +{ > >>> + tree fndecl; > >>> + > >>> + if (!is_gimple_call (stmt)) > >>> + return false; > >>> + > >>> + fndecl = gimple_call_fndecl (stmt); > >>> + if (fndecl && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL) > >>> + switch (DECL_FUNCTION_CODE (fndecl)) > >>> + { > >>> + case BUILT_IN_ALLOCA: > >>> + case BUILT_IN_ALLOCA_WITH_ALIGN: > >>> + return true; > >>> + } > >> > >> This should have failed bootstrap because of -Wswitch-enum. > >> You need > >> default: > >> break; > >> in. > >> > >>> + switch (DECL_FUNCTION_CODE (fndecl)) > >>> + { > >>> + case BUILT_IN_ALLOCA: > >>> + case BUILT_IN_ALLOCA_WITH_ALIGN: > >>> + return true; > >> > >> Likewise here. > >> > > Or write it in the more natural way as an if. > > > > I'm open for that suggestion. > > Then I should probably also rewrite the switch statement > in special_function_p as an if. I think a switch is a good fit though I don't mind an if as we probably know we'll never get more than two alloca builtins (heh, you never know). Richard. > Thanks > Bernd. > > > > > Bernd > > > >
On 07/21/16 13:35, Richard Biener wrote: > On Thu, 21 Jul 2016, Bernd Edlinger wrote: > >> On 07/21/16 13:25, Bernd Schmidt wrote: >>> >>> >>> On 07/21/2016 01:16 PM, Jakub Jelinek wrote: >>>> On Thu, Jul 21, 2016 at 11:04:48AM +0000, Bernd Edlinger wrote: >>>>> bool >>>>> +gimple_alloca_call_p (const gimple *stmt) >>>>> +{ >>>>> + tree fndecl; >>>>> + >>>>> + if (!is_gimple_call (stmt)) >>>>> + return false; >>>>> + >>>>> + fndecl = gimple_call_fndecl (stmt); >>>>> + if (fndecl && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL) >>>>> + switch (DECL_FUNCTION_CODE (fndecl)) >>>>> + { >>>>> + case BUILT_IN_ALLOCA: >>>>> + case BUILT_IN_ALLOCA_WITH_ALIGN: >>>>> + return true; >>>>> + } >>>> >>>> This should have failed bootstrap because of -Wswitch-enum. >>>> You need >>>> default: >>>> break; >>>> in. >>>> >>>>> + switch (DECL_FUNCTION_CODE (fndecl)) >>>>> + { >>>>> + case BUILT_IN_ALLOCA: >>>>> + case BUILT_IN_ALLOCA_WITH_ALIGN: >>>>> + return true; >>>> >>>> Likewise here. >>>> >>> Or write it in the more natural way as an if. >>> >> >> I'm open for that suggestion. >> >> Then I should probably also rewrite the switch statement >> in special_function_p as an if. > > I think a switch is a good fit though I don't mind an if as we probably > know we'll never get more than two alloca builtins (heh, you never know). > Thanks, style-nits are always welcome for me. I also do care about that a lot. I will keep the switch at the moment, and continue the boot-strap with the approved version. BTW: in the function below "is_tm_builtin" there is a single switch in a block statement, we usually don't do that redundant braces... Richard, do you still have objections against the builtin_setjmp patch from https://gcc.gnu.org/ml/gcc-patches/2016-07/msg01225.html ? Bernd. > Richard. > >> Thanks >> Bernd. >> >>> >>> Bernd >>> >> >> >
On Thu, 21 Jul 2016, Bernd Edlinger wrote: > On 07/21/16 13:35, Richard Biener wrote: > > On Thu, 21 Jul 2016, Bernd Edlinger wrote: > > > >> On 07/21/16 13:25, Bernd Schmidt wrote: > >>> > >>> > >>> On 07/21/2016 01:16 PM, Jakub Jelinek wrote: > >>>> On Thu, Jul 21, 2016 at 11:04:48AM +0000, Bernd Edlinger wrote: > >>>>> bool > >>>>> +gimple_alloca_call_p (const gimple *stmt) > >>>>> +{ > >>>>> + tree fndecl; > >>>>> + > >>>>> + if (!is_gimple_call (stmt)) > >>>>> + return false; > >>>>> + > >>>>> + fndecl = gimple_call_fndecl (stmt); > >>>>> + if (fndecl && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL) > >>>>> + switch (DECL_FUNCTION_CODE (fndecl)) > >>>>> + { > >>>>> + case BUILT_IN_ALLOCA: > >>>>> + case BUILT_IN_ALLOCA_WITH_ALIGN: > >>>>> + return true; > >>>>> + } > >>>> > >>>> This should have failed bootstrap because of -Wswitch-enum. > >>>> You need > >>>> default: > >>>> break; > >>>> in. > >>>> > >>>>> + switch (DECL_FUNCTION_CODE (fndecl)) > >>>>> + { > >>>>> + case BUILT_IN_ALLOCA: > >>>>> + case BUILT_IN_ALLOCA_WITH_ALIGN: > >>>>> + return true; > >>>> > >>>> Likewise here. > >>>> > >>> Or write it in the more natural way as an if. > >>> > >> > >> I'm open for that suggestion. > >> > >> Then I should probably also rewrite the switch statement > >> in special_function_p as an if. > > > > I think a switch is a good fit though I don't mind an if as we probably > > know we'll never get more than two alloca builtins (heh, you never know). > > > > Thanks, style-nits are always welcome for me. I also do care about > that a lot. > > I will keep the switch at the moment, and continue the boot-strap > with the approved version. > > BTW: in the function below "is_tm_builtin" there is a single switch > in a block statement, we usually don't do that redundant braces... > > > Richard, do you still have objections against the builtin_setjmp patch > from https://gcc.gnu.org/ml/gcc-patches/2016-07/msg01225.html ? No, I think it is ok, thus, approved as well. Good to see that special_function_p cleaned up ;) Thanks, Richard.
On 07/21/16 14:08, Richard Biener wrote: > On Thu, 21 Jul 2016, Bernd Edlinger wrote: > >> On 07/21/16 13:35, Richard Biener wrote: >>> On Thu, 21 Jul 2016, Bernd Edlinger wrote: >>> >>>> On 07/21/16 13:25, Bernd Schmidt wrote: >>>>> >>>>> >>>>> On 07/21/2016 01:16 PM, Jakub Jelinek wrote: >>>>>> On Thu, Jul 21, 2016 at 11:04:48AM +0000, Bernd Edlinger wrote: >>>>>>> bool >>>>>>> +gimple_alloca_call_p (const gimple *stmt) >>>>>>> +{ >>>>>>> + tree fndecl; >>>>>>> + >>>>>>> + if (!is_gimple_call (stmt)) >>>>>>> + return false; >>>>>>> + >>>>>>> + fndecl = gimple_call_fndecl (stmt); >>>>>>> + if (fndecl && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL) >>>>>>> + switch (DECL_FUNCTION_CODE (fndecl)) >>>>>>> + { >>>>>>> + case BUILT_IN_ALLOCA: >>>>>>> + case BUILT_IN_ALLOCA_WITH_ALIGN: >>>>>>> + return true; >>>>>>> + } >>>>>> >>>>>> This should have failed bootstrap because of -Wswitch-enum. >>>>>> You need >>>>>> default: >>>>>> break; >>>>>> in. >>>>>> >>>>>>> + switch (DECL_FUNCTION_CODE (fndecl)) >>>>>>> + { >>>>>>> + case BUILT_IN_ALLOCA: >>>>>>> + case BUILT_IN_ALLOCA_WITH_ALIGN: >>>>>>> + return true; >>>>>> >>>>>> Likewise here. >>>>>> >>>>> Or write it in the more natural way as an if. >>>>> >>>> >>>> I'm open for that suggestion. >>>> >>>> Then I should probably also rewrite the switch statement >>>> in special_function_p as an if. >>> >>> I think a switch is a good fit though I don't mind an if as we probably >>> know we'll never get more than two alloca builtins (heh, you never know). >>> >> >> Thanks, style-nits are always welcome for me. I also do care about >> that a lot. >> >> I will keep the switch at the moment, and continue the boot-strap >> with the approved version. >> >> BTW: in the function below "is_tm_builtin" there is a single switch >> in a block statement, we usually don't do that redundant braces... >> >> >> Richard, do you still have objections against the builtin_setjmp patch >> from https://gcc.gnu.org/ml/gcc-patches/2016-07/msg01225.html ? > > No, I think it is ok, thus, approved as well. Good to see that > special_function_p cleaned up ;) > Yeah that was about time! But that function is still like a mine field. And it looks like it has been waiting there for me, all these years ;) Jeff has found a reference to "savectx" in Solaris10, so it is probably not yet completely dead as I thought. If we need to keep the handling of savectx in special_function_p, that would mean there ought to be a new built-in function for savectx as well IMO. With that hint I have googled up a header file /usr/include/sys/proc.h from 2002 where the function signature can be seen: extern void savectx(kthread_t *); But that is in a #ifdef _KERNEL block, which means it is a kernel function. All that is only necessary, because we try to fix broken header files :( I mean they should simply add a __attribute__((returns_twice)) here. So I think that means that it is not possible to fix a kernel-header file with a fixinclude rule. I have heard that gcc can be built on solaris, but is gcc also used as a tool to generate the solaris kernel? Is it really justified to have a built-in function for that? I mean, that would be there even for windows and linux where that function name is not reserved? Thanks Bernd.
On 07/21/2016 09:07 AM, Bernd Edlinger wrote: > On 07/21/16 14:08, Richard Biener wrote: >> On Thu, 21 Jul 2016, Bernd Edlinger wrote: >> >>> On 07/21/16 13:35, Richard Biener wrote: >>>> On Thu, 21 Jul 2016, Bernd Edlinger wrote: >>>> >>>>> On 07/21/16 13:25, Bernd Schmidt wrote: >>>>>> >>>>>> >>>>>> On 07/21/2016 01:16 PM, Jakub Jelinek wrote: >>>>>>> On Thu, Jul 21, 2016 at 11:04:48AM +0000, Bernd Edlinger wrote: >>>>>>>> bool >>>>>>>> +gimple_alloca_call_p (const gimple *stmt) >>>>>>>> +{ >>>>>>>> + tree fndecl; >>>>>>>> + >>>>>>>> + if (!is_gimple_call (stmt)) >>>>>>>> + return false; >>>>>>>> + >>>>>>>> + fndecl = gimple_call_fndecl (stmt); >>>>>>>> + if (fndecl && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL) >>>>>>>> + switch (DECL_FUNCTION_CODE (fndecl)) >>>>>>>> + { >>>>>>>> + case BUILT_IN_ALLOCA: >>>>>>>> + case BUILT_IN_ALLOCA_WITH_ALIGN: >>>>>>>> + return true; >>>>>>>> + } >>>>>>> >>>>>>> This should have failed bootstrap because of -Wswitch-enum. >>>>>>> You need >>>>>>> default: >>>>>>> break; >>>>>>> in. >>>>>>> >>>>>>>> + switch (DECL_FUNCTION_CODE (fndecl)) >>>>>>>> + { >>>>>>>> + case BUILT_IN_ALLOCA: >>>>>>>> + case BUILT_IN_ALLOCA_WITH_ALIGN: >>>>>>>> + return true; >>>>>>> >>>>>>> Likewise here. >>>>>>> >>>>>> Or write it in the more natural way as an if. >>>>>> >>>>> >>>>> I'm open for that suggestion. >>>>> >>>>> Then I should probably also rewrite the switch statement >>>>> in special_function_p as an if. >>>> >>>> I think a switch is a good fit though I don't mind an if as we probably >>>> know we'll never get more than two alloca builtins (heh, you never know). >>>> >>> >>> Thanks, style-nits are always welcome for me. I also do care about >>> that a lot. >>> >>> I will keep the switch at the moment, and continue the boot-strap >>> with the approved version. >>> >>> BTW: in the function below "is_tm_builtin" there is a single switch >>> in a block statement, we usually don't do that redundant braces... >>> >>> >>> Richard, do you still have objections against the builtin_setjmp patch >>> from https://gcc.gnu.org/ml/gcc-patches/2016-07/msg01225.html ? >> >> No, I think it is ok, thus, approved as well. Good to see that >> special_function_p cleaned up ;) >> > > Yeah that was about time! But that function is still like a mine field. > And it looks like it has been waiting there for me, all these years ;) > > Jeff has found a reference to "savectx" in Solaris10, so it is probably > not yet completely dead as I thought. > > If we need to keep the handling of savectx in special_function_p, that > would mean there ought to be a new built-in function for savectx as > well IMO. > > With that hint I have googled up a header file /usr/include/sys/proc.h > from 2002 where the function signature can be seen: > > extern void savectx(kthread_t *); > > But that is in a #ifdef _KERNEL block, which means it is a kernel > function. It's possible the issue came up building the SunOS/Solaris kernel -- circa 1992 I was loosely involved in a project that was bringing up that kernel on non-Sun hardware. And that fits my recollection of when savectx support went into GCC. jeff
On 07/21/16 17:17, Jeff Law wrote: > It's possible the issue came up building the SunOS/Solaris kernel -- > circa 1992 I was loosely involved in a project that was bringing up that > kernel on non-Sun hardware. And that fits my recollection of when > savectx support went into GCC. > > jeff Yes. That matches to the svn revision where "savectx" appeared for the first time: r201 | rms | 1992-01-17 23:48:42 +0100 (Fr, 17. Jan 1992) | 2 Zeilen Geänderte Pfade: A /trunk/gcc/calls.c Initial revision So we had 25 years to fix a header file... Bernd.
2016-07-21 Bernd Edlinger <bernd.edlinger@hotmail.de> PR middle-end/71876 * calls.c (gimple_maybe_alloca_call_p): New function. Return true if STMT may be an alloca call. (gimple_alloca_call_p, alloca_call_p): Return only true for the builtin alloca call. * calls.h (gimple_maybe_alloca_call_p): New function. * tree-inline.c (inline_forbidden_p_stmt): Use gimple_maybe_alloca_call_p here. Index: gcc/calls.c =================================================================== --- gcc/calls.c (revision 238584) +++ gcc/calls.c (working copy) @@ -617,10 +617,10 @@ setjmp_call_p (const_tree fndecl) } -/* Return true if STMT is an alloca call. */ +/* Return true if STMT may be an alloca call. */ bool -gimple_alloca_call_p (const gimple *stmt) +gimple_maybe_alloca_call_p (const gimple *stmt) { tree fndecl; @@ -634,16 +634,44 @@ bool return false; } -/* Return true when exp contains alloca call. */ +/* Return true if STMT is a builtin alloca call. */ bool +gimple_alloca_call_p (const gimple *stmt) +{ + tree fndecl; + + if (!is_gimple_call (stmt)) + return false; + + fndecl = gimple_call_fndecl (stmt); + if (fndecl && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL) + switch (DECL_FUNCTION_CODE (fndecl)) + { + case BUILT_IN_ALLOCA: + case BUILT_IN_ALLOCA_WITH_ALIGN: + return true; + } + + return false; +} + +/* Return true when exp contains a builtin alloca call. */ + +bool alloca_call_p (const_tree exp) { tree fndecl; if (TREE_CODE (exp) == CALL_EXPR && (fndecl = get_callee_fndecl (exp)) - && (special_function_p (fndecl, 0) & ECF_MAY_BE_ALLOCA)) - return true; + && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL) + switch (DECL_FUNCTION_CODE (fndecl)) + { + case BUILT_IN_ALLOCA: + case BUILT_IN_ALLOCA_WITH_ALIGN: + return true; + } + return false; } Index: gcc/calls.h =================================================================== --- gcc/calls.h (revision 238584) +++ gcc/calls.h (working copy) @@ -23,6 +23,7 @@ along with GCC; see the file COPYING3. If not see extern int flags_from_decl_or_type (const_tree); extern int call_expr_flags (const_tree); extern int setjmp_call_p (const_tree); +extern bool gimple_maybe_alloca_call_p (const gimple *); extern bool gimple_alloca_call_p (const gimple *); extern bool alloca_call_p (const_tree); extern bool must_pass_in_stack_var_size (machine_mode, const_tree); Index: gcc/tree-inline.c =================================================================== --- gcc/tree-inline.c (revision 238584) +++ gcc/tree-inline.c (working copy) @@ -3577,7 +3577,7 @@ inline_forbidden_p_stmt (gimple_stmt_iterator *gsi RAM instead of 256MB. Don't do so for alloca calls emitted for VLA objects as those can't cause unbounded growth (they're always wrapped inside stack_save/stack_restore regions. */ - if (gimple_alloca_call_p (stmt) + if (gimple_maybe_alloca_call_p (stmt) && !gimple_call_alloca_for_var_p (as_a <gcall *> (stmt)) && !lookup_attribute ("always_inline", DECL_ATTRIBUTES (fn))) {