Jakub, thanks a lot for you review and comments.
> On Jul 19, 2018, at 12:31 PM, Jakub Jelinek <ja...@redhat.com> wrote: > > 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)? do you imply that we should only expand it as (int) ((unsigned char *)p)[n] - (int) ((unsigned char *)q)[n] when we are sure int type is wider than unsigned char? > 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. even on this targets, is char type still 8-bit? then int type is still wider than char? > 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. even when int type is as wide as char, expand it as (int) ((unsigned char *)p)[n] - (int) ((unsigned char *)q)[n] should still be correct (even though not optimal), doesn’t it? do I miss anything in this part? > > 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. thank you for raising this issue, Yes, I will update this part of the code as you suggested. Qing
