On Thu, Jul 19, 2018 at 11:49:16AM -0500, Qing Zhao wrote:
> As Wilco mentioned in PR78809 after I checked in the last part of
> implementation of inline strcmp:
>
> See http://www.iso-9899.info/n1570.html
> section 7.24.4:
>
> "The sign of a nonzero value returned by the comparison functions memcmp,
> strcmp, and strncmp is determined
> by the sign of the difference between the values of the first pair of
> characters (both interpreted as unsigned char)
> that differ in the objects being compared."
>
> currently, in my implementation, I used char type when expanding
> strcmp/strncmp, and unsigned char when expanding
> memcmp.
>
> from the C standard, we should use unsigned char for all
> strcmp/strncmp/memcmp.
>
> the change is quite simple, and I have tested it on X86, aarch64 and powerPC,
> no regressions.
>
> Okay for trunk?
If you expand it as (int) ((unsigned char *)p)[n] - (int) ((unsigned char
*)q)[n]
then aren't you relying on int type to have wider precision than unsigned char
(or unit_mode being narrower than mode)? I don't see anywhere where you'd
give up on doing the inline expansion on targets where e.g. lowest
addressable unit would be 16-bit and int would be 16-bit too.
On targets where int is as wide as char, one would need to expand it instead
as something like:
if (((unsigned char *)p)[n] == ((unsigned char *)q)[n]) loop;
ret = ((unsigned char *)p)[n] < ((unsigned char *)q)[n] ? -1 : 1;
or similar or just use the library routine.
Also:
var_rtx
= adjust_address (var_rtx_array, TYPE_MODE (unit_type_node), offset);
const_rtx = c_readstr (const_str + offset, unit_mode);
rtx op0 = (const_str_n == 1) ? const_rtx : var_rtx;
rtx op1 = (const_str_n == 1) ? var_rtx : const_rtx;
result = expand_simple_binop (mode, MINUS, op0, op1,
result, is_memcmp ? 1 : 0, OPTAB_WIDEN);
doesn't look correct to me, var_rtx and const_rtx here are in unit_mode,
you need to convert those to mode before you can use those in
expand_simple_binop, using
op0 = convert_modes (mode, unit_mode, op0, 1);
op1 = convert_modes (mode, unit_mode, op1, 1);
before the expand_simple_binop.
While expand_simple_binop is called with an unsignedp argument, that is
meant for the cases where the expansion needs to widen it further, not for
calling expand_simple_binop with arguments with known incorrect mode;
furthermore, one of them being CONST_INT which has VOIDmode.
> +2018-07-19 Qing Zhao <qing.z...@oracle.com>
> +
> + * builtins.c (expand_builtin_memcmp): Delete the last parameter for
> + call to inline_expand_builtin_string_cmp.
> + (expand_builtin_strcmp): Likewise.
> + (expand_builtin_strncmp): Likewise.
> + (inline_string_cmp): Delete the last parameter, change char_type_node
> + to unsigned_char_type_node for strcmp/strncmp;
> + (inline_expand_builtin_string_cmp): Delete the last parameter.
> +
Jakub
