Re: [PATCH v2 1/6] wcsmbs: Add wcscpy loop unroll option

["Gabriel F. T. Gomes" <gabriel@inconstante.eti.br>]

On Wed, Mar 13 2019, Adhemerval Zanella wrote:
> This allows an architecture the use the old generic implementation
> and also set explicit loop unrolling.

s/the use/to use/

> 	* include/loop_unroll.h: New file.
> 	* wcsmbs/wcscpy (__wcscpy): Add option to use loop unrolling
> 	besides generic implementation.

OK.

> +/* Loop unroll macro to be used for explicity force loop unrolling with a
> +   configurable number or iterations.  The idea is to make the loop unrolling
> +   independent of whether compiler is able to unrolling through specific
> +   optimizations options (-funroll-loops or -funroll-all-loops).
> +
> +   For instance, to implement strcpy with SRC being the source input and 
> +   DEST the destination buffer, it is expected the macro to be used in this
> +   way:
> +
> +     #define ITERATION(index)	\
> +       ({ char c = *str++; *dest++ = c; c != '\0' })
> +
> +     while (1)
> +       UNROLL_REPEAT (4, ITERATION)
> +
> +   The loop will be manually unrolled 4 times.  Another option is to do
> +   the index update after the tests:
> +
> +     #define ITERATION(index)	\
> +       ({ char c = *(str + index); *(dest + index) = c; c != '\0' })
> +     #define UPDATE(n)		\
> +       str += n; dst += n
> +
> +     while (1)
> +       UNROLL_REPEAT_UPDATE (4, ITERATION, UPDATE)
> +
> +   The loop will be manually unrolled 4 times and the SRC and DEST pointers
> +   will be update only after last iteration.
> +
> +   Currently both macros unrolls the loop 8 times at maximum.  */

Thanks for writing this detailed explanation.

> +#define UNROLL_REPEAT_1(X)    if (!X(0)) break;
> +#define UNROLL_REPEAT_2(X)    UNROLL_REPEAT_1(X) if (!X(1)) break;
> +#define UNROLL_REPEAT_3(X)    UNROLL_REPEAT_2(X) if (!X(2)) break;
> +#define UNROLL_REPEAT_4(X)    UNROLL_REPEAT_3(X) if (!X(3)) break;
> +#define UNROLL_REPEAT_5(X)    UNROLL_REPEAT_4(X) if (!X(4)) break;
> +#define UNROLL_REPEAT_6(X)    UNROLL_REPEAT_5(X) if (!X(5)) break;
> +#define UNROLL_REPEAT_7(X)    UNROLL_REPEAT_6(X) if (!X(6)) break;
> +#define UNROLL_REPEAT_8(X)    UNROLL_REPEAT_7(X) if (!X(7)) break;
                                                ^         ^
Missing space between macro call and parentheses (twice in each line).

> +#define UNROLL_REPEAT__(N, X) UNROLL_EXPAND(UNROLL_REPEAT_ ## N) (X)
                                              ^
I think that this is OK according to the style guidelines, as it just
expands the name to UNROLL_REPEAT_1, UNROLL_REPEAT_2, etc.  But maybe I
just didn't quite understand the guidelines.

> +#define UNROLL_REPEAT(N, X)                \
> +  (void) ({                                \
> +    UNROLL_REPEAT_(UNROLL_EXPAND(N), X);   \
                     ^
Missing space.

> +  /* Some architectures might have costly tail function call (powerpc
> +     for instance) where wmemcpy call overhead for smalls sizes might
> +     be costly than just unroll the main loop.  */

s/be costly than just unroll/be more costly than just unrolling/

... I think.

Looks good to me with these changes.

Reviewed-by: Gabriel F. T. Gomes <gabriel@inconstante.eti.br>