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

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Guido Falsi-7
On 02/03/20 18:13, Ryan Libby wrote:

> On Mon, Mar 2, 2020 at 12:45 AM Alexander V. Chernikov <[hidden email]> wrote:
>>
>> 28.02.2020, 18:32, "Ryan Libby" <[hidden email]>:
>>> 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.

Should I file a bug report on bugzilla?

--
Guido Falsi <[hidden email]>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-emulation
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

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

Guido Falsi-7
On 04/03/20 12:27, Guido Falsi wrote:

>
> 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.
>
> Should I file a bug report on bugzilla?
>

Adding:

.if ${.CURDIR:M*emulators/virtualbox-ose}
USE_GCC=9
.endif

to make.conf seems to be a good workaround.

Writing this here for the record.

--
Guido Falsi <[hidden email]>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-emulation
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

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

Ryan Libby
In reply to this post by Guido Falsi-7
On Wed, Mar 4, 2020 at 3:27 AM Guido Falsi <[hidden email]> wrote:

>
> On 02/03/20 18:13, Ryan Libby wrote:
> > On Mon, Mar 2, 2020 at 12:45 AM Alexander V. Chernikov <[hidden email]> wrote:
> >>
> >> 28.02.2020, 18:32, "Ryan Libby" <[hidden email]>:
> >>> 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.
>
> Should I file a bug report on bugzilla?
>
> --
> Guido Falsi <[hidden email]>

We've discussed whether to provide compatibility code for older compilers here:
https://reviews.freebsd.org/D23937

There's a bug tracking virtualbox-ose being pinned to an old compiler here:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=236616

I see you have commented on that bug.
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-emulation
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

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

Brooks Davis-2
In reply to this post by Guido Falsi-7
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 <[hidden email]> wrote:
> >>
> >> 28.02.2020, 18:32, "Ryan Libby" <[hidden email]>:
> >>> 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

signature.asc (465 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Ryan Libby
On Wed, Mar 4, 2020 at 10:28 AM Brooks Davis <[hidden email]> 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 <[hidden email]> wrote:
> > >>
> > >> 28.02.2020, 18:32, "Ryan Libby" <[hidden email]>:
> > >>> 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.
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-emulation
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

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

Kyle Evans-4
In reply to this post by Ryan Libby
On Wed, Mar 4, 2020 at 11:27 AM Ryan Libby <[hidden email]> wrote:

>
> On Wed, Mar 4, 2020 at 3:27 AM Guido Falsi <[hidden email]> wrote:
> >
> > On 02/03/20 18:13, Ryan Libby wrote:
> > > On Mon, Mar 2, 2020 at 12:45 AM Alexander V. Chernikov <[hidden email]> wrote:
> > >>
> > >> 28.02.2020, 18:32, "Ryan Libby" <[hidden email]>:
> > >>> 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.
> >
> > Should I file a bug report on bugzilla?
> >
> > --
> > Guido Falsi <[hidden email]>
>
> We've discussed whether to provide compatibility code for older compilers here:
> https://reviews.freebsd.org/D23937
>
> There's a bug tracking virtualbox-ose being pinned to an old compiler here:
> https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=236616
>
> I see you have commented on that bug.

I've opened https://reviews.freebsd.org/D23967 and PR 244603
specifically for coercing the port to use GCC9 instead, which does
indeed fix the breakage. Once that lands, I'll investigate 236616 some
more to see if newer llvm (including 10) is still broken here.

Thanks,

Kyle Evans
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-emulation
To unsubscribe, send any mail to "[hidden email]"