This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH][Middle-end][version 2]change char type to unsigned char type when expanding strcmp/strncmp


Hi,

this is the 2nd version of the change, mainly addressed Jakub’s comments:

1. Give up the inlining expansion for strcmp/strncmp/memcmp on a target
where the type of the call has same or narrower presicion than unsigned
char.
2.  add conversions before expand_simple_binop to the two operands.

and
3. also updated comments of routine inline_string_cmp to reflect the conversions
in the expanded code.

have tested on X86 and aarch64. No regressions.

Okay for thunk?

Qing

gcc/ChangeLog:

+2018-07-20  Qing Zhao  <qing.zhao@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, add conversions to the
+       two operands.
+       (inline_expand_builtin_string_cmp): Delete the last parameter, give up
+       the inlining expansion on target where the type of the call has same or 
+       narrower presicion than unsigned char.
+

Attachment: 78809C_uchar.patch
Description: Binary data


> On Jul 19, 2018, at 12:31 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> 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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]