Re: [PATCH] x86: use builtins to read eflags

[Bill Wendling]

On Wed, Dec 15, 2021 at 4:57 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> 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:
> linux-kernel@xxxxxxxxxxxxxxx
>
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