Re: [PATCH] don't declare header-defined functions both static and inline, pt 2
On Thu, 16 Feb 2023, Patrick Palka wrote:
> This fixes some header-defined functions that are undesirably declared
> static and weren't caught by the "^static inline" pattern used in the
> previous patch.
>
> gcc/ChangeLog:
>
> * hash-table.h (gt_pch_nx): Remove static.
> * lra-int.h (lra_change_class): Likewise.
> * recog.h (which_op_alt): Likewise.
> * sel-sched-ir.h (sel_bb_empty_or_nop_p): Replace static with
> inline.
I went ahead and pushed this since I reckon it's a fairly safe/obvious
follow-up to the main patch
(https://gcc.gnu.org/pipermail/gcc-patches/2023-February/612130.html).
> ---
> gcc/hash-table.h | 2 +-
> gcc/lra-int.h | 2 +-
> gcc/recog.h| 2 +-
> gcc/sel-sched-ir.h | 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/hash-table.h b/gcc/hash-table.h
> index 3f87ec06f37..c0c6e1cd83d 100644
> --- a/gcc/hash-table.h
> +++ b/gcc/hash-table.h
> @@ -1275,7 +1275,7 @@ hashtab_entry_note_pointers (void *obj, void *h,
> gt_pointer_operator op,
> }
>
> template
> -static void
> +void
> gt_pch_nx (hash_table *h)
> {
>h->check_complete_insertion ();
> diff --git a/gcc/lra-int.h b/gcc/lra-int.h
> index 73f8eb004b0..a400a0f85e2 100644
> --- a/gcc/lra-int.h
> +++ b/gcc/lra-int.h
> @@ -428,7 +428,7 @@ lra_get_regno_hard_regno (int regno)
>
> /* Change class of pseudo REGNO to NEW_CLASS. Print info about it
> using TITLE. Output a new line if NL_P. */
> -static void inline
> +inline void
> lra_change_class (int regno, enum reg_class new_class,
> const char *title, bool nl_p)
> {
> diff --git a/gcc/recog.h b/gcc/recog.h
> index 764fa90afde..539a27c3edf 100644
> --- a/gcc/recog.h
> +++ b/gcc/recog.h
> @@ -382,7 +382,7 @@ extern const operand_alternative *recog_op_alt;
> on operand OP of the current instruction alternative (which_alternative).
> Only valid after calling preprocess_constraints and constrain_operands.
> */
>
> -inline static const operand_alternative *
> +inline const operand_alternative *
> which_op_alt ()
> {
>gcc_checking_assert (IN_RANGE (which_alternative, 0,
> diff --git a/gcc/sel-sched-ir.h b/gcc/sel-sched-ir.h
> index 7034a1ab06c..0e87134c6db 100644
> --- a/gcc/sel-sched-ir.h
> +++ b/gcc/sel-sched-ir.h
> @@ -1096,7 +1096,7 @@ get_loop_exit_edges_unique_dests (const class loop
> *loop)
>return edges;
> }
>
> -static bool
> +inline bool
> sel_bb_empty_or_nop_p (basic_block bb)
> {
>insn_t first = sel_bb_head (bb), last;
> --
> 2.39.2.422.gc867e4fa18
>
>
Re: [PATCH] don't declare header-defined functions both static and inline
On Thu, Feb 16, 2023 at 08:37:34AM -0500, Patrick Palka wrote: > I can confirm that this patch only modifies headers that reside in > $prefix/lib/gcc/x86_64-pc-linux-gnu/13.0.1/plugin/include/ > (with --enable-languages=c,c++,fortran,objc,obj-c++ --enable-jit make install) > > Good point, I was able to scrape the functions modified by this patch > with the below shell script which outputs the name of each function that > has at least two overloads/definitions (possibly in different headers), > followed by the headers in which it's defined: > > I manually verified that none of these function definitions conflict > with each other. > Bootstrapping with objc enabled revealed a minor manual changed was > needed in gcc/objc/objc-act.cc to avoid a redeclaration mismatch error. > > -- >8 -- > > Subject: [PATCH] don't declare header-defined functions both static and inline > > Many functions defined in our headers are declared 'static inline' which > is a C idiom that predates GCC's move to C++ as the implementation > language. But in C++ the inline keyword is more than just a compiler > hint, and is sufficient to give the function the intended semantics. > In fact declaring a function both static and inline is a pessimization > since static effectively disables the desired definition merging > behavior enabled by inline, and is also a source of (harmless) ODR > violations when a static inline function gets called from a non-static > inline one (such as tree_operand_check calling tree_operand_length). > > This patch mechanically fixes the vast majority of occurrences of this > anti-pattern throughout the compiler's headers via the command line > > echo gcc/*.h gcc/*/*.h | xargs sed -i 's/^static inline/inline/g' > > The patch also manually removes the redundant declarations of is_ivar > and lookup_category in gcc/objc/objc-act.cc which would otherwise > conflict with those in objc-act.h (due to the difference in staticness). > > Besides fixing some ODR violations, this speeds up stage1 cc1plus by > about 2% and reduces the size of its text segment by 1.5MB. Thanks for doing the extra work, LGTM for trunk then. > gcc/ChangeLog: > > * addresses.h: Mechanically drop 'static' from 'static inline' > functions via s/^static inline/inline/g. > * asan.h: Likewise. > * attribs.h: Likewise. > * basic-block.h: Likewise. > * bitmap.h: Likewise. > * cfghooks.h: Likewise. > * cfgloop.h: Likewise. > * cgraph.h: Likewise. > * cselib.h: Likewise. > * data-streamer.h: Likewise. > * debug.h: Likewise. > * df.h: Likewise. > * diagnostic.h: Likewise. > * dominance.h: Likewise. > * dumpfile.h: Likewise. > * emit-rtl.h: Likewise. > * except.h: Likewise. > * expmed.h: Likewise. > * expr.h: Likewise. > * fixed-value.h: Likewise. > * gengtype.h: Likewise. > * gimple-expr.h: Likewise. > * gimple-iterator.h: Likewise. > * gimple-predict.h: Likewise. > * gimple-range-fold.h: Likewise. > * gimple-ssa.h: Likewise. > * gimple.h: Likewise. > * graphite.h: Likewise. > * hard-reg-set.h: Likewise. > * hash-map.h: Likewise. > * hash-set.h: Likewise. > * hash-table.h: Likewise. > * hwint.h: Likewise. > * input.h: Likewise. > * insn-addr.h: Likewise. > * internal-fn.h: Likewise. > * ipa-fnsummary.h: Likewise. > * ipa-icf-gimple.h: Likewise. > * ipa-inline.h: Likewise. > * ipa-modref.h: Likewise. > * ipa-prop.h: Likewise. > * ira-int.h: Likewise. > * ira.h: Likewise. > * lra-int.h: Likewise. > * lra.h: Likewise. > * lto-streamer.h: Likewise. > * memmodel.h: Likewise. > * omp-general.h: Likewise. > * optabs-query.h: Likewise. > * optabs.h: Likewise. > * plugin.h: Likewise. > * pretty-print.h: Likewise. > * range.h: Likewise. > * read-md.h: Likewise. > * recog.h: Likewise. > * regs.h: Likewise. > * rtl-iter.h: Likewise. > * rtl.h: Likewise. > * sbitmap.h: Likewise. > * sched-int.h: Likewise. > * sel-sched-ir.h: Likewise. > * sese.h: Likewise. > * sparseset.h: Likewise. > * ssa-iterators.h: Likewise. > * system.h: Likewise. > * target-globals.h: Likewise. > * target.h: Likewise. > * timevar.h: Likewise. > * tree-chrec.h: Likewise. > * tree-data-ref.h: Likewise. > * tree-iterator.h: Likewise. > * tree-outof-ssa.h: Likewise. > * tree-phinodes.h: Likewise. >
[PATCH] don't declare header-defined functions both static and inline, pt 2
This fixes some header-defined functions that are undesirably declared
static and weren't caught by the "^static inline" pattern used in the
previous patch.
gcc/ChangeLog:
* hash-table.h (gt_pch_nx): Remove static.
* lra-int.h (lra_change_class): Likewise.
* recog.h (which_op_alt): Likewise.
* sel-sched-ir.h (sel_bb_empty_or_nop_p): Replace static with
inline.
---
gcc/hash-table.h | 2 +-
gcc/lra-int.h | 2 +-
gcc/recog.h| 2 +-
gcc/sel-sched-ir.h | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/gcc/hash-table.h b/gcc/hash-table.h
index 3f87ec06f37..c0c6e1cd83d 100644
--- a/gcc/hash-table.h
+++ b/gcc/hash-table.h
@@ -1275,7 +1275,7 @@ hashtab_entry_note_pointers (void *obj, void *h,
gt_pointer_operator op,
}
template
-static void
+void
gt_pch_nx (hash_table *h)
{
h->check_complete_insertion ();
diff --git a/gcc/lra-int.h b/gcc/lra-int.h
index 73f8eb004b0..a400a0f85e2 100644
--- a/gcc/lra-int.h
+++ b/gcc/lra-int.h
@@ -428,7 +428,7 @@ lra_get_regno_hard_regno (int regno)
/* Change class of pseudo REGNO to NEW_CLASS. Print info about it
using TITLE. Output a new line if NL_P. */
-static void inline
+inline void
lra_change_class (int regno, enum reg_class new_class,
const char *title, bool nl_p)
{
diff --git a/gcc/recog.h b/gcc/recog.h
index 764fa90afde..539a27c3edf 100644
--- a/gcc/recog.h
+++ b/gcc/recog.h
@@ -382,7 +382,7 @@ extern const operand_alternative *recog_op_alt;
on operand OP of the current instruction alternative (which_alternative).
Only valid after calling preprocess_constraints and constrain_operands. */
-inline static const operand_alternative *
+inline const operand_alternative *
which_op_alt ()
{
gcc_checking_assert (IN_RANGE (which_alternative, 0,
diff --git a/gcc/sel-sched-ir.h b/gcc/sel-sched-ir.h
index 7034a1ab06c..0e87134c6db 100644
--- a/gcc/sel-sched-ir.h
+++ b/gcc/sel-sched-ir.h
@@ -1096,7 +1096,7 @@ get_loop_exit_edges_unique_dests (const class loop *loop)
return edges;
}
-static bool
+inline bool
sel_bb_empty_or_nop_p (basic_block bb)
{
insn_t first = sel_bb_head (bb), last;
--
2.39.2.422.gc867e4fa18
Re: [PATCH] don't declare header-defined functions both static and inline
> 3) we have also gcc/ada/gcc-interface/*.h with > ada.h:#define INLINE static inline > gigi.h:static inline unsigned HOST_WIDE_INT > gigi.h:static inline bool > gigi.h:static inline bool > gigi.h:static inline bool > gigi.h:static inline tree > gigi.h:static inline tree > gigi.h:static inline tree > gigi.h:static inline tree > gigi.h:static inline tree > gigi.h:static inline tree > gigi.h:static inline tree > gigi.h:static inline tree > gigi.h:static inline tree > gigi.h:static inline tree >I think we can defer that to Ada maintaners but we should tell them >about it Removing all the above "static" is OK. -- Eric Botcazou
Re: [PATCH] don't declare header-defined functions both static and inline
On Tue, Jan 31, 2023 at 9:43 AM Jakub Jelinek wrote: > > On Tue, Jan 31, 2023 at 08:05:15AM +0100, Richard Biener via Gcc-patches > wrote: > > On Tue, Jan 31, 2023 at 4:39 AM Patrick Palka via Gcc-patches > > wrote: > > > > > > Many functions defined in our headers are declared 'static inline' which > > > is a vestige from when GCC's implementation language was C. But in C++ > > > the inline keyword is more than just a compiler hint, and is sufficient > > > to give the function the intended semantics. In fact declaring a > > > (namespace-scope) function both static and inline is a pessimization > > > since static effectively disables the intended definition merging > > > behavior that inline provides, and is also a source of (harmless) ODR > > > violations when a static inline function gets called from a non-static > > > inline one (such as tree_operand_length being called from > > > tree_operand_check). > > > > > > This patch mechanically fixes the vast majority of occurrences of this > > > anti-pattern throughout the compiler's headers via the command line > > > > > > echo gcc/*.h gcc/*/*.h | xargs sed -i 's/^static inline/inline/g' > > > > > > Besides fixing the ODR violations, this speeds up stage1 cc1plus by > > > about 2% and reduces the size of its text segment by 1.5MB. > > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, would this be OK to > > > push now or wait for stage1? > > > > Speeding up and reducing the size of cc1plus improves on continued > > regression in this area. So I'd vote +1 to do this now, let's see what > > others > > think. > > I lean towards +1 too, but > 1) we should make sure we don't do that for installed headers with the >exception of headers meant for GCC plugins; in quick skimming I don't >see ginclude/ headers among the changed ones, but perhaps doing all >languages make install, then applying the patch, make and make install >again into another tree and compare all the headers in those tree >with the exception of paths with /plugin/include/ in it? > 2) we should make sure we don't introduce ODR violations through this, if >say 2 headers would define different static inline functions with the >same name (or even one static inline and another without that). >Don't know what would be best to catch that, get from the patch >list of changed functions and then search for it in readelf -wi output >using some script, or would LTO bootstrap detect that, or libabigail? >What I'm worried about is 2 different headers defining the same >function perhaps with different arguments/return values or content and >that we happen to never include the two headers in the same TU Ah, didn't think of that - yes, I suppose it might be that LTO bootstrap would warn about those? > 3) we have also gcc/ada/gcc-interface/*.h with > ada.h:#define INLINE static inline > gigi.h:static inline unsigned HOST_WIDE_INT > gigi.h:static inline bool > gigi.h:static inline bool > gigi.h:static inline bool > gigi.h:static inline tree > gigi.h:static inline tree > gigi.h:static inline tree > gigi.h:static inline tree > gigi.h:static inline tree > gigi.h:static inline tree > gigi.h:static inline tree > gigi.h:static inline tree > gigi.h:static inline tree > gigi.h:static inline tree >I think we can defer that to Ada maintaners but we should tell them >about it > 4) there are some static inline also in >gcc/config/*/*.h (and some in gcc/common/*/*.h - though in that case >it is solely in installed header that shouldn't be changed) > avr/avr-protos.h:static inline unsigned > pru/pru-protos.h:static inline bool > rs6000/rs6000-internal.h:static inline bool > rs6000/rs6000-protos.h:static inline bool > s390/s390-builtins.h:static inline unsigned int > s390/s390-builtins.h:static inline unsigned int > s390/vecintrin.h:static inline int >The last one is an installed header, so I think we shouldn't touch >it, the rest should be considered. The above probably asks to split the patch up and separate gcc/*/*.h into individual patches? Richard. > > Jakub >
Re: [PATCH] don't declare header-defined functions both static and inline
On Tue, Jan 31, 2023 at 08:05:15AM +0100, Richard Biener via Gcc-patches wrote: > On Tue, Jan 31, 2023 at 4:39 AM Patrick Palka via Gcc-patches > wrote: > > > > Many functions defined in our headers are declared 'static inline' which > > is a vestige from when GCC's implementation language was C. But in C++ > > the inline keyword is more than just a compiler hint, and is sufficient > > to give the function the intended semantics. In fact declaring a > > (namespace-scope) function both static and inline is a pessimization > > since static effectively disables the intended definition merging > > behavior that inline provides, and is also a source of (harmless) ODR > > violations when a static inline function gets called from a non-static > > inline one (such as tree_operand_length being called from > > tree_operand_check). > > > > This patch mechanically fixes the vast majority of occurrences of this > > anti-pattern throughout the compiler's headers via the command line > > > > echo gcc/*.h gcc/*/*.h | xargs sed -i 's/^static inline/inline/g' > > > > Besides fixing the ODR violations, this speeds up stage1 cc1plus by > > about 2% and reduces the size of its text segment by 1.5MB. > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu, would this be OK to > > push now or wait for stage1? > > Speeding up and reducing the size of cc1plus improves on continued > regression in this area. So I'd vote +1 to do this now, let's see what others > think. I lean towards +1 too, but 1) we should make sure we don't do that for installed headers with the exception of headers meant for GCC plugins; in quick skimming I don't see ginclude/ headers among the changed ones, but perhaps doing all languages make install, then applying the patch, make and make install again into another tree and compare all the headers in those tree with the exception of paths with /plugin/include/ in it? 2) we should make sure we don't introduce ODR violations through this, if say 2 headers would define different static inline functions with the same name (or even one static inline and another without that). Don't know what would be best to catch that, get from the patch list of changed functions and then search for it in readelf -wi output using some script, or would LTO bootstrap detect that, or libabigail? What I'm worried about is 2 different headers defining the same function perhaps with different arguments/return values or content and that we happen to never include the two headers in the same TU 3) we have also gcc/ada/gcc-interface/*.h with ada.h:#define INLINE static inline gigi.h:static inline unsigned HOST_WIDE_INT gigi.h:static inline bool gigi.h:static inline bool gigi.h:static inline bool gigi.h:static inline tree gigi.h:static inline tree gigi.h:static inline tree gigi.h:static inline tree gigi.h:static inline tree gigi.h:static inline tree gigi.h:static inline tree gigi.h:static inline tree gigi.h:static inline tree gigi.h:static inline tree I think we can defer that to Ada maintaners but we should tell them about it 4) there are some static inline also in gcc/config/*/*.h (and some in gcc/common/*/*.h - though in that case it is solely in installed header that shouldn't be changed) avr/avr-protos.h:static inline unsigned pru/pru-protos.h:static inline bool rs6000/rs6000-internal.h:static inline bool rs6000/rs6000-protos.h:static inline bool s390/s390-builtins.h:static inline unsigned int s390/s390-builtins.h:static inline unsigned int s390/vecintrin.h:static inline int The last one is an installed header, so I think we shouldn't touch it, the rest should be considered. Jakub
