lkml.org 
[lkml]   [2022]   [Mar]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v5] x86: use builtins to read eflags
On Tue, Mar 1, 2022 at 12:19 PM Bill Wendling <morbo@google.com> wrote:
>
Bump for review.

-bw

> This issue arose due to Clang's issue with the "=rm" constraint. Clang
> chooses to be conservative in these situations and always uses memory
> instead of registers, resulting in sub-optimal code. (This is a known
> issue, which is currently being addressed.)
>
> This function has gone through numerous changes over the years:
>
> - The original version of this function used the "=g" constraint,
> which has the following description:
>
> Any register, memory or immediate integer operand is allowed,
> except for registers that are not general registers.
>
> - This was changed to "=r" in commit f1f029c7bfbf ("x86: fix assembly
> constraints in native_save_fl()"), because someone noticed that:
>
> the offset of the flags variable from the stack pointer will
> change when the pushf is performed. gcc doesn't attempt to
> understand that fact, and address used for pop will still be the
> same. It will write to somewhere near flags on the stack but not
> actually into it and overwrite some other value.
>
> - However, commit f1f029c7bfbf ("x86: fix assembly constraints in
> native_save_fl()") was partially reverted in commit ab94fcf528d1
> ("x86: allow "=rm" in native_save_fl()"), because the original
> reporter of the issue was using a broken x86 simulator. The
> justification for this change was:
>
> "=rm" is allowed in this context, because "pop" is explicitly
> defined to adjust the stack pointer *before* it evaluates its
> effective address, if it has one. Thus, we do end up writing to
> the correct address even if we use an on-stack memory argument.
>
> Clang generates good code when the builtins are used. On one benchmark,
> a hotspot in kmem_cache_free went from using 5.18% of cycles popping to
> a memory address to 0.13% popping to a register. This benefit is
> magnified given that this code is inlined in numerous places in the
> kernel.
>
> The builtins also help GCC. It allows GCC (and Clang) to reduce register
> pressure and, consequently, register spills by rescheduling
> instructions. It can't happen with instructions in inline assembly,
> because compilers view inline assembly blocks as "black boxes," whose
> instructions can't be rescheduled.
>
> Another benefit of builtins over asm blocks is that compilers are able
> to make more precise inlining decisions, since they no longer need to
> rely on imprecise measures based on newline counts.
>
> A trivial example demonstrates this code motion.
>
> void y(void);
> unsigned long x(void) {
> unsigned long v = __builtin_ia32_readeflags_u64();
> y();
> return v;
> }
>
> GCC at -O1:
> pushq %rbx
> pushfq
> popq %rbx
> movl $0, %eax
> call y
> movq %rbx, %rax
> popq %rbx
> ret
>
> GCC at -O2:
> pushq %r12
> pushfq
> xorl %eax, %eax
> popq %r12
> call y
> movq %r12, %rax
> popq %r12
> ret
>
> Link: https://gist.github.com/nickdesaulniers/b4d0f6e26f8cbef0ae4c5352cfeaca67
> Link: https://github.com/llvm/llvm-project/issues/20571
> Link: https://gcc.gnu.org/onlinedocs/gcc/Simple-Constraints.html#Simple-Constraints
> Link: https://godbolt.org/z/5n3Eov1xT
> Signed-off-by: Bill Wendling <morbo@google.com>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> v5: - Incorporate Nick's suggestion to limit the change to Clang >= 14.0 and
> GCC.
> v4: - Clang now no longer generates stack frames when using these builtins.
> - Corrected misspellings.
> v3: - Add blurb indicating that GCC's output hasn't changed.
> v2: - Kept the original function to retain the out-of-line symbol.
> - Improved the commit message.
> - Note that I couldn't use Nick's suggestion of
>
> return IS_ENABLED(CONFIG_X86_64) ? ...
>
> because Clang complains about using __builtin_ia32_readeflags_u32 in
> 64-bit mode.
> ---
> arch/x86/include/asm/irqflags.h | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h
> index 87761396e8cc..2eded855f6ab 100644
> --- a/arch/x86/include/asm/irqflags.h
> +++ b/arch/x86/include/asm/irqflags.h
> @@ -19,6 +19,11 @@
> extern inline unsigned long native_save_fl(void);
> extern __always_inline unsigned long native_save_fl(void)
> {
> +#if defined(CC_IS_CLANG) && defined(UNWINDER_ORC) && CLANG_VERSION < 140000
> + /*
> + * Clang forced frame pointers via the builtins until Clang-14. Use
> + * this as a fall-back until the minimum Clang version is >= 14.0.
> + */
> unsigned long flags;
>
> /*
> @@ -33,6 +38,11 @@ extern __always_inline unsigned long native_save_fl(void)
> : "memory");
>
> return flags;
> +#elif defined(CONFIG_X86_64)
> + return __builtin_ia32_readeflags_u64();
> +#else
> + return __builtin_ia32_readeflags_u32();
> +#endif
> }
>
> static __always_inline void native_irq_disable(void)
> --
> 2.35.1.574.g5d30c73bfb-goog
>

\
 
 \ /
  Last update: 2022-03-15 00:08    [W:2.123 / U:0.592 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site