This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH][Middle-end]change char type to unsigned char type when expanding strcmp/strncmp
- From: Qing Zhao <qing dot zhao at oracle dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: gcc Patches <gcc-patches at gcc dot gnu dot org>, jeff Law <law at redhat dot com>, richard Biener <rguenther at suse dot de>
- Date: Thu, 19 Jul 2018 14:06:16 -0500
- Subject: Re: [PATCH][Middle-end]change char type to unsigned char type when expanding strcmp/strncmp
- References: <F0A510E7-248E-4EAF-81D4-1688B8C947B6@oracle.com> <20180719173100.GU7166@tucnak>
thanks a lot for you review and comments.
> On Jul 19, 2018, at 12:31 PM, Jakub Jelinek <firstname.lastname@example.org> 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
>> 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?
> = 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.