Messages in this thread | | | From | Bill Wendling <> | Date | Thu, 27 Jan 2022 12:56:09 -0800 | Subject | Re: [PATCH v2] x86: use builtins to read eflags |
| |
Bump for review.
On Tue, Dec 28, 2021 at 6:13 PM Bill Wendling <morbo@google.com> wrote: > > GCC and Clang both have builtins to read and write the EFLAGS register. > This allows the compiler to determine the best way to generate this > code, which can improve code generation. > > This issue arose due to Clang's issue with the "=rm" constraint. Clang > chooses to be conservative in these situations, and so uses memory > instead of registers. This is a known issue, which is currently being > addressed. > > However, using builtins is benefiical in general, because it removes the > burden of determining what's the way to read the flags register from the > programmer and places it on to the compiler, which has the information > needed to make that decision. Indeed, this piece of code has had several > changes over the years, some of which were pinging back and forth to > determine the correct constraints to use. > > With this change, Clang generates better code: > > Original code: > movq $0, -48(%rbp) > #APP > # __raw_save_flags > pushfq > popq -48(%rbp) > #NO_APP > movq -48(%rbp), %rbx > > New code: > pushfq > popq %rbx > #APP > > Note that the stack slot in the original code is no longer needed in the > new code, saving a small amount of stack space. > > Signed-off-by: Bill Wendling <morbo@google.com> > --- > 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 | 19 +++++-------------- > 1 file changed, 5 insertions(+), 14 deletions(-) > > diff --git a/arch/x86/include/asm/irqflags.h b/arch/x86/include/asm/irqflags.h > index c5ce9845c999..27f919ea7ac3 100644 > --- a/arch/x86/include/asm/irqflags.h > +++ b/arch/x86/include/asm/irqflags.h > @@ -19,20 +19,11 @@ > extern inline unsigned long native_save_fl(void); > extern __always_inline unsigned long native_save_fl(void) > { > - unsigned long flags; > - > - /* > - * "=rm" is safe here, because "pop" adjusts the stack before > - * it evaluates its effective address -- this is part of the > - * documented behavior of the "pop" instruction. > - */ > - asm volatile("# __raw_save_flags\n\t" > - "pushf ; pop %0" > - : "=rm" (flags) > - : /* no input */ > - : "memory"); > - > - return flags; > +#ifdef CONFIG_X86_64 > + return __builtin_ia32_readeflags_u64(); > +#else > + return __builtin_ia32_readeflags_u32(); > +#endif > } > > static __always_inline void native_irq_disable(void) > -- > 2.34.1.448.ga2b2bfdf31-goog >
| |