Re: [PATCH v7 2/5] x86/asm: add volatile, cl obbers and zero-length check in inline memcmp

On July 23, 2026 5:35:02 PM PDT, Mauricio Faria de Oliveira <[email protected]> wrote:
>On 2026-07-23 20:12, H. Peter Anvin wrote:
>> On 2026-07-22 23:59, Jan Beulich wrote:
>>>> 
>>>> Also, this is silly. Instead of adding a whole separate test, just do "test %3,%3" before the repe to set ZF and let the REPE skip.
>>> 
>>> Besides this, isn't the function effectively returning bool wrong anyway? This
>>> way you can use it for equal / not-equal comparisons, but not for sorting and
>>> alike.
>>> 
>> There is no use case in the early code for sorting, and it seems rather broken
>> to burden the code with that.
>
>If this is merged, other users not in early code might eventually
>appear, which might introduce the use case for sorting. Even though that
>is unlikely, as such users could probably use regular memcmp() instead,
>consider that just in case, so I can ask for your input/advice on this:
>
>> That being said it probably should return bool explicitly (it makes no sense
>> for the prototype to be different than the internal variable.)
>> 
>> We could call it memneq() if someone really, really cares, I guess.
>> 
>> The early code is very size-sensitive, so I'm really not fond of the idea of
>> burdening it further. Perhaps something like:
>
>Do you think this implementation (returns -1/0/+1) is reasonable,
>size-wise, for early code?
>
>I considered submitting it eventually, once the return value difference
>to regular memcmp() was called out [0], as an improvement, if there's
>agreement this would be a good idea considering the scenario above.
>
>static __always_inline int __inline_memcmp(const void *s1, const void
>*s2, size_t len)
>{
>        int above, below;
>
>        asm volatile("test %2, %2\n\t"
>                     "repe cmpsb"
>                     : "+S" (s1), "+D" (s2), "+c" (len),
>                       "=@cca" (above), "=@ccb" (below)
>                     : : "memory");
>
>        return above - below;
>}
>
>@ arch/x86/boot/string.o
>00000028 <memcmp>:
>  28:   66 57                   push   %di
>  2a:   66 56                   push   %si
>  2c:   66 89 c6                mov    %ax,%si
>  2f:   66 89 d7                mov    %dx,%di
>
>  32:   66 85 c9                test   %cx,%cx
>  35:   f3 a6                   repz cmpsb %es:(%edi),%ds:(%esi)
>  37:   0f 97 c0                seta   %al
>  3a:   66 0f b6 c0             movzbw %al,%ax
>  3e:   0f 92 c2                setb   %dl
>  41:   66 0f b6 d2             movzbw %dl,%dx
>  45:   66 29 d0                sub    %dx,%ax
>
>  48:   66 5e                   pop    %si
>  4a:   66 5f                   pop    %di
>  4c:   66 c3                   retw
>
>> A memory clobber is ugly here since no memory is actually modified, although
>
>I'm definitely not an expert, but IIUIC, this memory clobber is for
>reads, not writes? Say, a caller/optimized code that writes to the
>buffer(s) prior to __inline_memcmp() and data might still reside in
>registers; even if theoretical/unlikely.
>
>As in [1]:
>
>  The "memory" clobber tells the compiler that the assembly code
>performs memory reads or writes [...] (for example, accessing the memory
>pointed to [...]). To ensure memory contains correct values, GCC may
>need to flush specific register values to memory before executing the
>asm.
>
>> it probably doesn't affect code; "cc" is completely redundant with condition
>> code output operand.
>
>Thanks for explaining. I'll remove that in the next version.
>
>[0]
>https://lore.kernel.org/all/[email protected]/
>[1]
>https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Clobbers-and-Scratch-Registers-1
>
>cheers,
>
>> static __always_inline bool
>> __inline_memcmp(const void *s1, const void *s2, size_t len)
>> {
>> 	bool diff;
>> 
>> 	if (__builtin_constant_p(len == 0)) {
>> 		if (!len)
>> 			return false;
>> 		asm volatile("repe cmpsb"
>> 				: "=@ccnz" (diff),
>> 				  "+D" (s1), "+S" (s2), "+c" (len)
>> 				: : "memory");
>> 	} else {
>> 		/* Clear ZF beforehand in case len == 0 */
>> 		asm volatile("test %3,%3; repe cmpsb"
>> 				: "=@ccnz" (diff),
>> 				  "+D" (s1), "+S" (s2), "+c" (len)
>> 				: : "memory");
>> 	}
>> 	return diff;
>> }
>
>

It's pretty bloaty; I don't know exactly how much the rest of the code bloats.

I would be fine with having mem[n]eq() though as an explicit API. For the out-of-line version used for compiler-generated memcmp() calls it doesn't matter. 

Here is a very compact way the out-of-line version can be written (showing the 64-bit version; formatting apologies, on a phone):

xor %eax,%eax    /* Sets ZF */
repe cmpsb
jr z,1f
movzbl -1(%rdi),%edi
mov -1(%rsi),%al
sub %edi,%eax
1: ret