2021-12-16 19:55:27

by Bill Wendling

[permalink] [raw]
Subject: Re: [PATCH] x86: use builtins to read eflags

On Wed, Dec 15, 2021 at 4:57 PM Thomas Gleixner <[email protected]> wrote:
>
> Bill,
>
> On Wed, Dec 15 2021 at 13:18, Bill Wendling wrote:
>
> please always CC the relevant mailing lists, i.e. this lacks a cc:
> [email protected]
>
I thought that was automatically added. But as Peter pointed out in
another email thread, no one has time to read the LKML, so it seems a
bit pointless? Nonetheless it's added now.

> > GCC and Clang both have builtins to read from and write to the
> > EFLAGS register. This allows the compiler to determine the best way
> > to generate the code, which can improve code generation.
>
> Emphasis on *can*. Just claiming that this might improve things does not
> cut it. Where is the prove?
>
There are a few proofs. First, clang generates better code with the
builtin. Yes, that's because clang doesn't handle the "=rm" constraint
in the same way that GCC does, but that's not really relevant (sure,
clang should correct this, but that shouldn't prevent this patch from
going, because builtins are generally better than inline assembly).
Builtins exist for a reason. The compiler's able to understand what's
going on and generate the appropriate code for it. It also gives the
compiler more freedom for optimizations.

Secondly, this one small function has had multiple changes since its
creation, basically pinging back and forth trying to determine the
best constraints to use:

6abcd98f x86: irqflags consolidation
f1f029c7 x86: fix assembly constraints in native_save_fl()
ab94fcf5 x86: allow "=rm" in native_save_fl()

The information on which form to use already exists in the compiler.
Using the builtin will save future churning and thus developers' time.

> IIRC, this was proposed before and the real reason was not better code
> generation but to address the confusion of clang vs. the '=rm'
> constraint which is still correct despite some clang folks having
> different opinions.
>
> So what has changed since then?

The minimal version of GCC is now 5.1, which supports these builtins.
That wasn't the case before.

> AFAICT, nothing. So I consider this as
> another attempt of "let's see whether it sticks".
>
The first patch was dismissed primarily because it was deemed too
convoluted, because I was trying to get past the then GCC minimal
version not supporting the builtins.

-bw


2021-12-17 12:49:03

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [PATCH] x86: use builtins to read eflags

On Thu, Dec 16, 2021 at 11:55:12AM -0800, Bill Wendling wrote:
> On Wed, Dec 15, 2021 at 4:57 PM Thomas Gleixner <[email protected]> wrote:
> >
> > Bill,
> >
> > On Wed, Dec 15 2021 at 13:18, Bill Wendling wrote:
> >
> > please always CC the relevant mailing lists, i.e. this lacks a cc:
> > [email protected]
> >
> I thought that was automatically added. But as Peter pointed out in
> another email thread, no one has time to read the LKML, so it seems a
> bit pointless? Nonetheless it's added now.

It is archived and really handy for future references. If/when needed etc..

2021-12-17 19:39:50

by Thomas Gleixner

[permalink] [raw]
Subject: Re: [PATCH] x86: use builtins to read eflags

Bill,

On Thu, Dec 16 2021 at 11:55, Bill Wendling wrote:
> On Wed, Dec 15, 2021 at 4:57 PM Thomas Gleixner <[email protected]> wrote:
>> Emphasis on *can*. Just claiming that this might improve things does not
>> cut it. Where is the prove?
>>
> There are a few proofs. First, clang generates better code with the
> builtin.

which is best demonstrated by showing the before and after.

> Yes, that's because clang doesn't handle the "=rm" constraint
> in the same way that GCC does, but that's not really relevant (sure,
> clang should correct this, but that shouldn't prevent this patch from
> going, because builtins are generally better than inline assembly).
> Builtins exist for a reason. The compiler's able to understand what's
> going on and generate the appropriate code for it. It also gives the
> compiler more freedom for optimizations.
>
> Secondly, this one small function has had multiple changes since its
> creation, basically pinging back and forth trying to determine the
> best constraints to use:
>
> 6abcd98f x86: irqflags consolidation
> f1f029c7 x86: fix assembly constraints in native_save_fl()
> ab94fcf5 x86: allow "=rm" in native_save_fl()
>
> The information on which form to use already exists in the compiler.
> Using the builtin will save future churning and thus developers' time.

Why is the above and this

> The minimal version of GCC is now 5.1, which supports these builtins.
> That wasn't the case before.

not part of the change log to avoid maintainers having to ask exactly
these questions?

Thanks,

tglx





2022-03-15 12:09:38

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH] x86: use builtins to read eflags

On 12/16/21 11:55, Bill Wendling wrote:
> On Wed, Dec 15, 2021 at 4:57 PM Thomas Gleixner <[email protected]> wrote:
>>
>> Bill,
>>
>> On Wed, Dec 15 2021 at 13:18, Bill Wendling wrote:
>>
>> please always CC the relevant mailing lists, i.e. this lacks a cc:
>> [email protected]
>>
> I thought that was automatically added. But as Peter pointed out in
> another email thread, no one has time to read the LKML, so it seems a
> bit pointless? Nonetheless it's added now.
>

Consider linux-kernel a distributed archive. Noone reads it in real
time, but it is really great to be able to search for someone specific
in one place.

-hpa

2022-03-15 23:48:48

by Bill Wendling

[permalink] [raw]
Subject: Re: [PATCH] x86: use builtins to read eflags

On Mon, Mar 14, 2022 at 4:10 PM H. Peter Anvin <[email protected]> wrote:
>
> On 12/16/21 11:55, Bill Wendling wrote:
> > On Wed, Dec 15, 2021 at 4:57 PM Thomas Gleixner <[email protected]> wrote:
> >>
> >> Bill,
> >>
> >> On Wed, Dec 15 2021 at 13:18, Bill Wendling wrote:
> >>
> >> please always CC the relevant mailing lists, i.e. this lacks a cc:
> >> [email protected]
> >>
> > I thought that was automatically added. But as Peter pointed out in
> > another email thread, no one has time to read the LKML, so it seems a
> > bit pointless? Nonetheless it's added now.
> >
>
> Consider linux-kernel a distributed archive. Noone reads it in real
> time, but it is really great to be able to search for someone specific
> in one place.
>
Ah! Okay. That makes more sense then. :-)

-bw