On Sun, 23 Feb 2025 13:52:15 +0100 Mattias Rönnblom <[email protected]> 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 <[email protected]> > --- 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.
