| Message ID | 87ob6wp8oh.fsf@talisman.default |
|---|---|
| State | New |
| Headers | show |
On Fri, Oct 11, 2013 at 9:29 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > This is a follow-up to: > > http://gcc.gnu.org/ml/gcc/2013-10/msg00075.html > > The outcome on IRC was that __asm ought to behave like aliases eventually, > so we should abandom the renaming-in-C thing. One way would be to code > the functions directly as asm files, or use inline asm, but it seems a > shame to do that when GCC already knows how to generate the function > body we want. > > This patch instead compiles the functions under a different name > and postprocesses the asm output. See the comment in the patch > for more details. > > It would be possible to do the whole compilation using pipes, > but that wouldn't pick up failures in earlier commands (at least > not without relying on bashisms). > > Tested on mips64-linux-gnu. OK to install? Btw, one other way of deliberately and forever get around GCC getting what you are after is using a toplevel asm to build the alias manually. Like asm(".alias __sync_synchronize sync_synchronize"); at least unless Honza starts to implement a (toplevel) asm parser to catch symbol definitions and uses (something we need a way for). Don't forget to mark sync_synchronize with the used attribute. Richard. > Thanks, > Richard > > > libgcc/ > * Makefile.in (sync_compile): New macro. > ($(libgcc-sync-size-funcs-o), $(libgcc-sync-funcs-o)) > ($(libgcc-sync-size-funcs-s-o), $(libgcc-sync-funcs-s-o)): Use it. > * sync.c: Remove aliases. Rename all functions to the_function. > > Index: libgcc/Makefile.in > =================================================================== > --- libgcc/Makefile.in 2013-10-11 08:18:40.309571904 +0100 > +++ libgcc/Makefile.in 2013-10-11 08:25:26.513967865 +0100 > @@ -718,38 +718,49 @@ libgcc-sync-size-funcs := $(foreach pref > $(foreach suffix, 1 2 4 8 16, \ > $(prefix)_$(suffix))) > > +# sync.c uses GCC's expansion of built-in functions to define out-of-line > +# versions of those same functions. This cannot be expressed directly in C, > +# even with GNU extensions, so instead sync.c calls the out-of-line function > +# "the_function" and we postprocess the assembly code to rename it. > +# > +# This code is morally assembly code rather than C code (and so cannot > +# be included in LTO, for example). However, having GCC do most of the > +# work saves recoding the functions as inline asm or .S files. > +sync_compile = $(gcc_compile_bare) $(compile_deps) $(SYNC_CFLAGS) -S \ > + -Wno-missing-prototypes $(1) -o $@.s.in && \ > + sed 's/the_function/__$*/g' < $@.s.in > $@.s && \ > + $(gcc_compile_bare) -c $@.s -o $@ && \ > + rm $@.s.in $@.s > + > libgcc-sync-size-funcs-o = $(patsubst %,%$(objext),$(libgcc-sync-size-funcs)) > $(libgcc-sync-size-funcs-o): %$(objext): $(srcdir)/sync.c > - $(gcc_compile) $(SYNC_CFLAGS) \ > + $(call sync_compile, \ > -DFN=`echo "$*" | sed 's/_[^_]*$$//'` \ > -DSIZE=`echo "$*" | sed 's/.*_//'` \ > - -c $< $(vis_hide) > + $< $(vis_hide)) > + > libgcc-objects += $(libgcc-sync-size-funcs-o) > > libgcc-sync-funcs := sync_synchronize > > libgcc-sync-funcs-o = $(patsubst %,%$(objext),$(libgcc-sync-funcs)) > $(libgcc-sync-funcs-o): %$(objext): $(srcdir)/sync.c > - $(gcc_compile) $(SYNC_CFLAGS) \ > - -DL$* \ > - -c $< $(vis_hide) > + $(call sync_compile, -DL$* $< $(vis_hide)) > libgcc-objects += $(libgcc-sync-funcs-o) > > ifeq ($(enable_shared),yes) > libgcc-sync-size-funcs-s-o = $(patsubst %,%_s$(objext), \ > $(libgcc-sync-size-funcs)) > $(libgcc-sync-size-funcs-s-o): %_s$(objext): $(srcdir)/sync.c > - $(gcc_s_compile) $(SYNC_CFLAGS) \ > + $(call sync_compile, \ > -DFN=`echo "$*" | sed 's/_[^_]*$$//'` \ > -DSIZE=`echo "$*" | sed 's/.*_//'` \ > - -c $< > + $< -DSHARED) > libgcc-s-objects += $(libgcc-sync-size-funcs-s-o) > > libgcc-sync-funcs-s-o = $(patsubst %,%_s$(objext),$(libgcc-sync-funcs)) > $(libgcc-sync-funcs-s-o): %_s$(objext): $(srcdir)/sync.c > - $(gcc_s_compile) $(SYNC_CFLAGS) \ > - -DL$* \ > - -c $< > + $(call sync_compile, -DL$* $< -DSHARED) > libgcc-s-objects += $(libgcc-sync-funcs-s-o) > endif > endif > Index: libgcc/sync.c > =================================================================== > --- libgcc/sync.c 2013-10-11 08:18:40.321572005 +0100 > +++ libgcc/sync.c 2013-10-11 08:18:40.664574880 +0100 > @@ -72,22 +72,22 @@ Software Foundation; either version 3, o > TYPE is a type that has UNITS bytes. */ > > #define DEFINE_V_PV(NAME, UNITS, TYPE) \ > - static TYPE \ > - NAME##_##UNITS (TYPE *ptr, TYPE value) \ > + TYPE \ > + the_function (TYPE *ptr, TYPE value) \ > { \ > return __##NAME (ptr, value); \ > } > > -#define DEFINE_V_PVV(NAME, UNITS, TYPE) \ > - static TYPE \ > - NAME##_##UNITS (TYPE *ptr, TYPE value1, TYPE value2) \ > +#define DEFINE_V_PVV(NAME, UNITS, TYPE) \ > + TYPE \ > + the_function (TYPE *ptr, TYPE value1, TYPE value2) \ > { \ > return __##NAME (ptr, value1, value2); \ > } > > #define DEFINE_BOOL_PVV(NAME, UNITS, TYPE) \ > - static _Bool \ > - NAME##_##UNITS (TYPE *ptr, TYPE value1, TYPE value2) \ > + _Bool \ > + the_function (TYPE *ptr, TYPE value1, TYPE value2) \ > { \ > return __##NAME (ptr, value1, value2); \ > } > @@ -118,9 +118,7 @@ #define local_sync_lock_test_and_set DEF > #define DEFINE1(NAME, UNITS, TYPE) \ > static int unused[sizeof (TYPE) == UNITS ? 1 : -1] \ > __attribute__((unused)); \ > - local_##NAME (NAME, UNITS, TYPE); \ > - typeof (NAME##_##UNITS) __##NAME##_##UNITS \ > - __attribute__((alias (#NAME "_" #UNITS))); > + local_##NAME (NAME, UNITS, TYPE) > > /* As above, but performing macro expansion on the arguments. */ > #define DEFINE(NAME, UNITS, TYPE) DEFINE1 (NAME, UNITS, TYPE) > @@ -167,13 +165,11 @@ DEFINE (FN, 8, UOItype) > > #if defined Lsync_synchronize > > -static void > -sync_synchronize (void) > +void > +the_function (void) > { > __sync_synchronize (); > } > -typeof (sync_synchronize) __sync_synchronize \ > - __attribute__((alias ("sync_synchronize"))); > > #endif >
On Fri, Oct 11, 2013 at 10:17:41AM +0200, Richard Biener wrote:
> asm(".alias __sync_synchronize sync_synchronize");
It is .set, but not everywhere.
/* The MIPS assembler has different syntax for .set. We set it to
.dummy to trap any errors. */
#undef SET_ASM_OP
#define SET_ASM_OP "\t.dummy\t"
But perhaps it would require fewer variants than providing inline asm
of the __sync_* builtin by hand for all the targets that need it.
Jakub
Jakub Jelinek <jakub@redhat.com> writes: > On Fri, Oct 11, 2013 at 10:17:41AM +0200, Richard Biener wrote: >> asm(".alias __sync_synchronize sync_synchronize"); > > It is .set, but not everywhere. > /* The MIPS assembler has different syntax for .set. We set it to > .dummy to trap any errors. */ > #undef SET_ASM_OP > #define SET_ASM_OP "\t.dummy\t" > But perhaps it would require fewer variants than providing inline asm > of the __sync_* builtin by hand for all the targets that need it. Yeah, that's why I prefer the sed approach. GCC knows how to do exactly what we want, and what syntax to use. We just need to take its output and change the function name. And like Richard says, parsing top-level asms would be fair game, especially if GCC and binutils ever were integrated (for libgccjit.so). So using top-level asms seems like putting off the inevitable (albeit putting it off further than __asm renaming). Do either of you object to the sed thing? Thanks, Richard
On Fri, Oct 11, 2013 at 11:43 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Jakub Jelinek <jakub@redhat.com> writes: >> On Fri, Oct 11, 2013 at 10:17:41AM +0200, Richard Biener wrote: >>> asm(".alias __sync_synchronize sync_synchronize"); >> >> It is .set, but not everywhere. >> /* The MIPS assembler has different syntax for .set. We set it to >> .dummy to trap any errors. */ >> #undef SET_ASM_OP >> #define SET_ASM_OP "\t.dummy\t" >> But perhaps it would require fewer variants than providing inline asm >> of the __sync_* builtin by hand for all the targets that need it. > > Yeah, that's why I prefer the sed approach. GCC knows how to do exactly > what we want, and what syntax to use. We just need to take its output and > change the function name. > > And like Richard says, parsing top-level asms would be fair game, > especially if GCC and binutils ever were integrated (for libgccjit.so). > So using top-level asms seems like putting off the inevitable > (albeit putting it off further than __asm renaming). > > Do either of you object to the sed thing? Well, ideally there would be a way to not lie about symbol names to GCC. That is, have a "native" way of telling GCC what to do here (which is as far as I understand to emit the code for the builtin-handled $SYM in a function with $SYM - provide the out-of-line copy for a builtin). For the __sync functions it's unfortunate that the library function has the same 'name' as the builtin and the builtin doesn't have an alternate spelling. So - can't we just add __builtin__sync_... spellings and use __sync_synchronize () { __builtin_sync_syncronize (); } ? (what if __builtin_sync_syncronize expands to a libcall? if it can't, what's the point of the library function?) Why does a simple __sync_synchronize () { __sync_synchronize (); } not work? On x86_64 I get from that: __sync_synchronize: .LFB0: .cfi_startproc mfence ret .cfi_endproc (similar to how you can have a definition of memcpy () and have another memcpy inside it inline-expanded) Richard. > Thanks, > Richard
On Fri, Oct 11, 2013 at 12:16:03PM +0200, Richard Biener wrote: > For the __sync functions it's unfortunate that the library function has > the same 'name' as the builtin and the builtin doesn't have an alternate > spelling. So - can't we just add __builtin__sync_... spellings and use > > __sync_synchronize () > { > __builtin_sync_syncronize (); > } > > ? (what if __builtin_sync_syncronize expands to a libcall? if it can't, > what's the point of the library function?) Actually, we already have a different spelling for that, __sync_synchronize () should be equivalent to __atomic_thread_fence (__ATOMIC_SEQ_CST) though no idea what exactly it does on targets I'm not familiar with. Jakub
Richard Biener <richard.guenther@gmail.com> writes: > On Fri, Oct 11, 2013 at 11:43 AM, Richard Sandiford > <rdsandiford@googlemail.com> wrote: >> Jakub Jelinek <jakub@redhat.com> writes: >>> On Fri, Oct 11, 2013 at 10:17:41AM +0200, Richard Biener wrote: >>>> asm(".alias __sync_synchronize sync_synchronize"); >>> >>> It is .set, but not everywhere. >>> /* The MIPS assembler has different syntax for .set. We set it to >>> .dummy to trap any errors. */ >>> #undef SET_ASM_OP >>> #define SET_ASM_OP "\t.dummy\t" >>> But perhaps it would require fewer variants than providing inline asm >>> of the __sync_* builtin by hand for all the targets that need it. >> >> Yeah, that's why I prefer the sed approach. GCC knows how to do exactly >> what we want, and what syntax to use. We just need to take its output and >> change the function name. >> >> And like Richard says, parsing top-level asms would be fair game, >> especially if GCC and binutils ever were integrated (for libgccjit.so). >> So using top-level asms seems like putting off the inevitable >> (albeit putting it off further than __asm renaming). >> >> Do either of you object to the sed thing? > > Well, ideally there would be a way to not lie about symbol names to GCC. > That is, have a "native" way of telling GCC what to do here (which is > as far as I understand to emit the code for the builtin-handled $SYM > in a function with $SYM - provide the out-of-line copy for a builtin). > > For the __sync functions it's unfortunate that the library function has > the same 'name' as the builtin and the builtin doesn't have an alternate > spelling. So - can't we just add __builtin__sync_... spellings and use > > __sync_synchronize () > { > __builtin_sync_syncronize (); > } > > ? (what if __builtin_sync_syncronize expands to a libcall? if it can't, > what's the point of the library function?) It can't expand to a libcall in nomips16 mode. It always expands to a libcall in mips16 mode. The point is to provide out-of-line nomips16 functions that the mips16 code can call. > Why does a simple > > __sync_synchronize () > { > __sync_synchronize (); > } > > not work? On x86_64 I get from that: > > __sync_synchronize: > .LFB0: > .cfi_startproc > mfence > ret > .cfi_endproc > > (similar to how you can have a definition of memcpy () and have > another memcpy inside it inline-expanded) Is that with optimisation enabled? -O2 gives me: __sync_synchronize: .LFB0: .cfi_startproc .p2align 4,,10 .p2align 3 .L2: jmp .L2 .cfi_endproc .LFE0: We do want to compile this stuff with optimisation enabled. Thanks, Richard
Index: libgcc/Makefile.in =================================================================== --- libgcc/Makefile.in 2013-10-11 08:18:40.309571904 +0100 +++ libgcc/Makefile.in 2013-10-11 08:25:26.513967865 +0100 @@ -718,38 +718,49 @@ libgcc-sync-size-funcs := $(foreach pref $(foreach suffix, 1 2 4 8 16, \ $(prefix)_$(suffix))) +# sync.c uses GCC's expansion of built-in functions to define out-of-line +# versions of those same functions. This cannot be expressed directly in C, +# even with GNU extensions, so instead sync.c calls the out-of-line function +# "the_function" and we postprocess the assembly code to rename it. +# +# This code is morally assembly code rather than C code (and so cannot +# be included in LTO, for example). However, having GCC do most of the +# work saves recoding the functions as inline asm or .S files. +sync_compile = $(gcc_compile_bare) $(compile_deps) $(SYNC_CFLAGS) -S \ + -Wno-missing-prototypes $(1) -o $@.s.in && \ + sed 's/the_function/__$*/g' < $@.s.in > $@.s && \ + $(gcc_compile_bare) -c $@.s -o $@ && \ + rm $@.s.in $@.s + libgcc-sync-size-funcs-o = $(patsubst %,%$(objext),$(libgcc-sync-size-funcs)) $(libgcc-sync-size-funcs-o): %$(objext): $(srcdir)/sync.c - $(gcc_compile) $(SYNC_CFLAGS) \ + $(call sync_compile, \ -DFN=`echo "$*" | sed 's/_[^_]*$$//'` \ -DSIZE=`echo "$*" | sed 's/.*_//'` \ - -c $< $(vis_hide) + $< $(vis_hide)) + libgcc-objects += $(libgcc-sync-size-funcs-o) libgcc-sync-funcs := sync_synchronize libgcc-sync-funcs-o = $(patsubst %,%$(objext),$(libgcc-sync-funcs)) $(libgcc-sync-funcs-o): %$(objext): $(srcdir)/sync.c - $(gcc_compile) $(SYNC_CFLAGS) \ - -DL$* \ - -c $< $(vis_hide) + $(call sync_compile, -DL$* $< $(vis_hide)) libgcc-objects += $(libgcc-sync-funcs-o) ifeq ($(enable_shared),yes) libgcc-sync-size-funcs-s-o = $(patsubst %,%_s$(objext), \ $(libgcc-sync-size-funcs)) $(libgcc-sync-size-funcs-s-o): %_s$(objext): $(srcdir)/sync.c - $(gcc_s_compile) $(SYNC_CFLAGS) \ + $(call sync_compile, \ -DFN=`echo "$*" | sed 's/_[^_]*$$//'` \ -DSIZE=`echo "$*" | sed 's/.*_//'` \ - -c $< + $< -DSHARED) libgcc-s-objects += $(libgcc-sync-size-funcs-s-o) libgcc-sync-funcs-s-o = $(patsubst %,%_s$(objext),$(libgcc-sync-funcs)) $(libgcc-sync-funcs-s-o): %_s$(objext): $(srcdir)/sync.c - $(gcc_s_compile) $(SYNC_CFLAGS) \ - -DL$* \ - -c $< + $(call sync_compile, -DL$* $< -DSHARED) libgcc-s-objects += $(libgcc-sync-funcs-s-o) endif endif Index: libgcc/sync.c =================================================================== --- libgcc/sync.c 2013-10-11 08:18:40.321572005 +0100 +++ libgcc/sync.c 2013-10-11 08:18:40.664574880 +0100 @@ -72,22 +72,22 @@ Software Foundation; either version 3, o TYPE is a type that has UNITS bytes. */ #define DEFINE_V_PV(NAME, UNITS, TYPE) \ - static TYPE \ - NAME##_##UNITS (TYPE *ptr, TYPE value) \ + TYPE \ + the_function (TYPE *ptr, TYPE value) \ { \ return __##NAME (ptr, value); \ } -#define DEFINE_V_PVV(NAME, UNITS, TYPE) \ - static TYPE \ - NAME##_##UNITS (TYPE *ptr, TYPE value1, TYPE value2) \ +#define DEFINE_V_PVV(NAME, UNITS, TYPE) \ + TYPE \ + the_function (TYPE *ptr, TYPE value1, TYPE value2) \ { \ return __##NAME (ptr, value1, value2); \ } #define DEFINE_BOOL_PVV(NAME, UNITS, TYPE) \ - static _Bool \ - NAME##_##UNITS (TYPE *ptr, TYPE value1, TYPE value2) \ + _Bool \ + the_function (TYPE *ptr, TYPE value1, TYPE value2) \ { \ return __##NAME (ptr, value1, value2); \ } @@ -118,9 +118,7 @@ #define local_sync_lock_test_and_set DEF #define DEFINE1(NAME, UNITS, TYPE) \ static int unused[sizeof (TYPE) == UNITS ? 1 : -1] \ __attribute__((unused)); \ - local_##NAME (NAME, UNITS, TYPE); \ - typeof (NAME##_##UNITS) __##NAME##_##UNITS \ - __attribute__((alias (#NAME "_" #UNITS))); + local_##NAME (NAME, UNITS, TYPE) /* As above, but performing macro expansion on the arguments. */ #define DEFINE(NAME, UNITS, TYPE) DEFINE1 (NAME, UNITS, TYPE) @@ -167,13 +165,11 @@ DEFINE (FN, 8, UOItype) #if defined Lsync_synchronize -static void -sync_synchronize (void) +void +the_function (void) { __sync_synchronize (); } -typeof (sync_synchronize) __sync_synchronize \ - __attribute__((alias ("sync_synchronize"))); #endif