Re: [RFC 1/1] eal/x86: replace inline assembly prefetch with cc builtins

[Stephen Hemminger <stephen@networkplumber.org>]

On Sun, 23 Feb 2025 13:52:15 +0100
Mattias Rönnblom <mattias.ronnblom@ericsson.com> wrote:

> Implement the 32- and 64-bit x86 rte_prefetch<n>() functions and
> rte_prefetch_non_temporal() using compiler builtins, rather than
> inline assembly.
> 
> This change frees the compiler to do certain optimizations that
> otherwise wouldn't be possible.
> 
> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> ---

I like this getting rid of asm is good.
AI review did spot some things worth addressing, I don't think benchmarking
is needed. But moving to arch neutral file is good idea.

Subject: Re: [RFC 1/1] eal/x86: replace inline assembly prefetch with cc builtins

The patch still applies cleanly to main. Codegen checked with gcc 13
at -O0 and -O2, x86_64 and -m32: locality 3/2/1/0 emits
prefetcht0/t1/t2/nta, identical to the current inline asm. Worth
resending as a non-RFC after the items below.

Warning

The comment in rte_prefetch_non_temporal() is a copy/paste leftover.
It says "1 sets target cache level to L3" but the call passes 0,
which GCC maps to prefetchnta.

The same four-line comment is repeated in every function. The
locality argument is also not a cache level; the mapping to
prefetcht0/t1/t2/nta is the x86 backend's choice. Replace with a
single comment above rte_prefetch0(), e.g.:

  /*
   * On x86, __builtin_prefetch() read hints with locality 3, 2, 1, 0
   * generate prefetcht0, prefetcht1, prefetcht2 and prefetchnta.
   */

The commit message says the change "frees the compiler to do certain
optimizations" without naming them. Say what inline asm blocks
(it is opaque to the optimizer; the pointer must be materialized and
loops containing it do not vectorize) and include either a codegen
example or benchmark numbers. Also state that the generated
instruction is unchanged.

Info

With this change the non-MSVC x86 code is the same as
lib/eal/loongarch/include/rte_prefetch.h. Consider providing the
__builtin_prefetch() versions in generic/rte_prefetch.h and letting
arch headers override only where needed (MSVC, arm, ppc). That drops
duplicate code in two arch headers.

Unlike the asm, __builtin_prefetch() emits nothing on 32-bit x86
when SSE is not enabled (e.g. -march=i686). Not a problem for DPDK
builds given the SSE4.2 baseline, but applications compiling the
headers with their own flags would silently lose the prefetch.