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

[Nick Desaulniers <ndesaulniers@google.com>]

On Wed, Dec 15, 2021 at 4:57 PM Thomas Gleixner <tglx@linutronix.de> 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@vger.kernel.org
>
> It's not that hard to figure that out because it's well documented.

For example, for every patch I send, I _always_ do:

$ git format-patch HEAD~
$ ./scripts/checkpatch.pl <patchfile>
$ ./scripts/get_maintainer.pl <patchfile>

Then put the above in my invocation of `git send-email` with
maintainers explicitly in --to and lists+everyone else on --cc.

> > 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?

At the least, I expect the compiler *could* re-schedule the
instructions in the pair, which it *cant* do for inline asm. Whether
it *would* or if that's a win performance wise...*shrug*.  I'd expect
the estimated code size to be more accurate, too.

>
> 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? AFAICT, nothing. So I consider this as
> another attempt of "let's see whether it sticks".

I wouldn't resend this kernel change until the builtins were fixed
first.  I think Reid and Phoebe's suggestion in:
https://reviews.llvm.org/D92695#inline-1086936
is probably a good start?

Once fixed, say in clang-14, the next question becomes "are we trading
sub-optimal codegen for worse codegen with previously released
versions of clang?"

Otherwise for "rm" I suspect that the code I read recently is
partially related to the issue at hand.
https://github.com/llvm/llvm-project/issues/20571#issuecomment-992965798
See this comment in source:
https://github.com/llvm/llvm-project/blob/9043c3d65b11b442226015acfbf8167684586cfa/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp#L5002-L5003

IIRC, I think you (Bill) mentioned somewhere before that we might need
a new TargetLowering::ConstraintType, perhaps something like
`TargetLowering::C_Register_Or_Memory`?  I suspect that the
conservative approach of "m" being chosen over "r" in "rm" is to avoid
register exhaustion during register allocation.  I'm not sure if the
`TargetLowering::ConstraintType` or `TargetLowering::AsmOperandInfo`
makes it through to register allocation, but I'd imagine if they did
then we'd first try to allocate these to a register, and if there was
later register pressure, we could then consider these as a possible
spill candidate, evicting them from a dedicated physical register.

Take that with a grain of salt; while that sounds nice, it's not often
I've touched register allocation!  I've also only really studied the
greedy register allocator.

On Thu, Dec 16, 2021 at 11:55 AM Bill Wendling <morbo@google.com> wrote:
> The minimal version of GCC is now 5.1, which supports these builtins.
> That wasn't the case before.

That's worthwhile to put in the commit message.
-- 
Thanks,
~Nick Desaulniers