Re: [PATCH v2.0] Use saturated arithmetic for overflow detection.

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Fri, 1 Nov 2013, Ondrej Bilka wrote:

> This version adds saturated arithmetic support with optimized x86_64
> version.

I consider such an x86_64 version to be premature optimization without 
clear benchmark results (speed or code size) to justify it.  It would not 
surprise me if use of the overflow flag is slow in some cases and straight 
comparisons would be faster.  And using these asm versions prevents the 
compiler from optimizing based on constant arguments.

In my view, we shouldn't optimize this with inline asm at all.  For any 
optimizations, work on getting appropriate built-in functions into GCC 
(making sure that they do get optimized there based on constant arguments, 
so that the overflow flag is used only when it's the most efficient 
approach) and then use those functions in glibc (architecture-independent 
file) conditional on the GCC version.

> +#define MUL_S(__x, __y)({ \
> +  size_t x = (__x), y = (__y);						\

No, you've got things the wrong way round here.

The macro parameters should not have leading underscores.

The variables defined within the macro, whose names appear in the 
replacement text, should have leading underscores (even better, names such 
as __mul_s_x and __mul_s_y to avoid possible conflicts if the calling code 
in glibc also uses leading underscores).

Though when you use an inline function, all this becomes irrelevant....

> +static inline size_t ADD_S(size_t x, size_t y)

Incorrect coding stype (missing newline before ADD_S, missing space before 
'(').

> +{
> +  size_t ret;
> +  asm ("add %%rdx, %%rax; jno 1f; xor %%rax, %%rax; not %%rax; 1:" : "=a" (ret) : "a" (x) , "d" (y));

Line too long.  Doesn't appear to allow for x32.  If you use built-in 
functions, of course you don't need to worry about x32 at all; that's all 
taken care of by GCC knowing what type the inline function is meant to 
saturate for.

-- 
Joseph S. Myers
joseph@codesourcery.com