svn commit: r358439 - head/sys/amd64/include

[Ryan Libby]

On Wed, Mar 4, 2020 at 10:28 AM Brooks Davis <brooks at freebsd.org> wrote:
>
> On Wed, Mar 04, 2020 at 12:27:08PM +0100, Guido Falsi wrote:
> > On 02/03/20 18:13, Ryan Libby wrote:
> > > On Mon, Mar 2, 2020 at 12:45 AM Alexander V. Chernikov <melifaro at ipfw.ru> wrote:
> > >>
> > >> 28.02.2020, 18:32, "Ryan Libby" <rlibby at freebsd.org>:
> > >>> Author: rlibby
> > >>> Date: Fri Feb 28 18:32:36 2020
> > >>> New Revision: 358439
> > >>> URL: https://svnweb.freebsd.org/changeset/base/358439
> > >>>
> > >>> Log:
> > >>>   amd64 atomic.h: minor codegen optimization in flag access
> > >>>
> > >>>   Previously the pattern to extract status flags from inline assembly
> > >>>   blocks was to use setcc in the block to write the flag to a register.
> > >>>   This was suboptimal in a few ways:
> > >>>    - It would lead to code like: sete %cl; test %cl; jne, i.e. a flag
> > >>>      would just be loaded into a register and then reloaded to a flag.
> > >>>    - The setcc would force the block to use an additional register.
> > >>>    - If the client code didn't care for the flag value then the setcc
> > >>>      would be entirely pointless but could not be eliminated by the
> > >>>      optimizer.
> > >>>
> > >>>   A more modern inline asm construct (since gcc 6 and clang 9) allows for
> > >> This effectively restricts kernel builds by all older compilers.
> > >> Is there any chance of making it conditional depending on the compiler version/features?
> > >
> > > Yes, it is possible to test for __GCC_ASM_FLAG_OUTPUTS__.  It is more
> > > maintenance effort going forward.  If building current with an old cross
> > > compiler is an important scenario, we can either revert this and the
> > > following revision or work up a patch to make it conditional.  I'll see
> > > what that might look like.
> > >
> >
> > Actually this causes emulators/virtualbox-ose port to fail to build:
> >
> > In file included from /usr/src/sys/sys/systm.h:44:
> > /usr/include/machine/atomic.h:230:1: error: invalid output constraint
> > '=@cce' in asm
> > ATOMIC_CMPSET(char);
> > ^
> > /usr/include/machine/atomic.h:205:4: note: expanded from macro
> > 'ATOMIC_CMPSET'
> >         : "=@cce" (res),                /* 0 */         \
> >           ^
> > /usr/include/machine/atomic.h:230:1: error: invalid output constraint
> > '=@cce' in asm
> >
> > (and so on)
> >
> >
> > the virtualbox-ose port is forced to use an older clang version due to
> > crashes when compiled with newer ones.
> >
> > Not sure whose responsibility is to fix this.
>
> I suspect that now that we don't care about gcc 4.2.1, we should
> restructure machine/atomic.h to use __atomic compiler builtins in nearly
> all cases.  We could then conditionalize small sets of mircooptimized
> assembly versions based on the availability of compiler features if they
> add any value.
>
> On CheriBSD we've switched the RISC-V to use the C versions and are
> overdue to do the same to MIPS.  Reworking things to make this the
> default would decrease our maintenance burden and it seems unlikely that
> most of our platforms would benefit from handcode assembly (given the
> general level of optimization in our lower-tier platforms).
>
> -- Brooks

There's further discussion on that topic in the original review (D23869)
and in D23661.