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

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

[Qing Zhao]

[-- Attachment #1: Type: text/plain, Size: 1277 bytes --]

Hi, 

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?

Qing

gcc/ChangeLog:

+2018-07-19  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;
+       (inline_expand_builtin_string_cmp): Delete the last parameter.
+

[-- Attachment #2: 78809C_uchar.patch --]
[-- Type: application/octet-stream, Size: 4560 bytes --]

From 1d87dd8e390d47802d70dee07f1f9c37572c660a Mon Sep 17 00:00:00 2001
From: qing zhao <qing.zhao@oracle.com>
Date: Thu, 19 Jul 2018 09:01:11 -0700
Subject: [PATCH] Change char to unsigned char for strcmp/strncmp when expand
 them to a sequence of byte comparisons.

Due to C standard 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.

bootstraped and tested on both X86 and Aarch64. no regression.
---
 gcc/builtins.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/gcc/builtins.c b/gcc/builtins.c
index c069d66..6f58384 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -119,7 +119,7 @@ static rtx expand_builtin_next_arg (void);
 static rtx expand_builtin_va_start (tree);
 static rtx expand_builtin_va_end (tree);
 static rtx expand_builtin_va_copy (tree);
-static rtx inline_expand_builtin_string_cmp (tree, rtx, bool);
+static rtx inline_expand_builtin_string_cmp (tree, rtx);
 static rtx expand_builtin_strcmp (tree, rtx);
 static rtx expand_builtin_strncmp (tree, rtx, machine_mode);
 static rtx builtin_memcpy_read_str (void *, HOST_WIDE_INT, scalar_int_mode);
@@ -4472,7 +4472,7 @@ expand_builtin_memcmp (tree exp, rtx target, bool result_eq)
 
   if (!result_eq && fcode != BUILT_IN_BCMP && no_overflow)
     {
-      result = inline_expand_builtin_string_cmp (exp, target, true);
+      result = inline_expand_builtin_string_cmp (exp, target);
       if (result)
 	return result;
     }
@@ -4551,7 +4551,7 @@ expand_builtin_strcmp (tree exp, ATTRIBUTE_UNUSED rtx target)
 
   /* Due to the performance benefit, always inline the calls first.  */
   rtx result = NULL_RTX;
-  result = inline_expand_builtin_string_cmp (exp, target, false);
+  result = inline_expand_builtin_string_cmp (exp, target);
   if (result)
     return result;
 
@@ -4670,7 +4670,7 @@ expand_builtin_strncmp (tree exp, ATTRIBUTE_UNUSED rtx target,
 
   /* Due to the performance benefit, always inline the calls first.  */
   rtx result = NULL_RTX;
-  result = inline_expand_builtin_string_cmp (exp, target, false);
+  result = inline_expand_builtin_string_cmp (exp, target);
   if (result)
     return result;
 
@@ -6778,8 +6778,7 @@ expand_builtin_goacc_parlevel_id_size (tree exp, rtx target, int ignore)
 static rtx
 inline_string_cmp (rtx target, tree var_str, const char *const_str,
 		   unsigned HOST_WIDE_INT length,
-		   int const_str_n, machine_mode mode,
-		   bool is_memcmp)
+		   int const_str_n, machine_mode mode)
 {
   HOST_WIDE_INT offset = 0;
   rtx var_rtx_array
@@ -6788,7 +6787,7 @@ inline_string_cmp (rtx target, tree var_str, const char *const_str,
   rtx const_rtx = NULL_RTX;
   rtx result = target ? target : gen_reg_rtx (mode);
   rtx_code_label *ne_label = gen_label_rtx ();
-  tree unit_type_node = is_memcmp ? unsigned_char_type_node : char_type_node;
+  tree unit_type_node = unsigned_char_type_node;
   scalar_int_mode unit_mode
     = as_a <scalar_int_mode> TYPE_MODE (unit_type_node);
 
@@ -6803,7 +6802,7 @@ inline_string_cmp (rtx target, tree var_str, const char *const_str,
       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);
+				    result, 1, OPTAB_WIDEN);
       if (i < length - 1)
 	emit_cmp_and_jump_insns (result, CONST0_RTX (mode), NE, NULL_RTX,
 	    			 mode, true, ne_label);
@@ -6822,7 +6821,7 @@ inline_string_cmp (rtx target, tree var_str, const char *const_str,
    TARGET if that's convenient.
    If the call is not been inlined, return NULL_RTX.  */
 static rtx
-inline_expand_builtin_string_cmp (tree exp, rtx target, bool is_memcmp)
+inline_expand_builtin_string_cmp (tree exp, rtx target)
 {
   tree fndecl = get_callee_fndecl (exp);
   enum built_in_function fcode = DECL_FUNCTION_CODE (fndecl);
@@ -6879,7 +6878,7 @@ inline_expand_builtin_string_cmp (tree exp, rtx target, bool is_memcmp)
   /* Now, start inline expansion the call.  */
   return inline_string_cmp (target, (const_str_n == 1) ? arg2 : arg1,
 			    (const_str_n == 1) ? src_str1 : src_str2, length,
-			    const_str_n, mode, is_memcmp);
+			    const_str_n, mode);
 }
 
 /* Expand an expression EXP that calls a built-in function,
-- 
1.9.1

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

[Jakub Jelinek]

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.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;
> +       (inline_expand_builtin_string_cmp): Delete the last parameter.
> +

	Jakub

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

[Qing Zhao]

Jakub,

thanks a lot for you review and comments.

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

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

[Jakub Jelinek]

On Thu, Jul 19, 2018 at 02:06:16PM -0500, Qing Zhao 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)?
> 
> 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? 

Yes.

> >  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?

C requires that int is at least 16-bit wide, so the sizeof (int) == sizeof
(char) case is only possible say with 16-bit char and 16-bit int, or 32-bit
char and 32-bit int etc.

> > 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?

No.  Consider p[n] being e.g. 1 and q[n] being __SCHAR_MAX__ + 3U and 16-bit
int and 16-bit char.  Then (unsigned char) 0x0001 < (unsigned char) 0x8002,
so it should return a negative number.  But (int) (0x0001U - 0x8002U) is
0x7fff, which is a positive int.  Now, if int is 17-bit and char is 16-bit,
this works fine, because is then -0x8001 and thus negative.

The above really works only if int is at least one bit wider than unsigned
char.

	Jakub

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

[Qing Zhao]

> On Jul 19, 2018, at 2:24 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> On Thu, Jul 19, 2018 at 02:06:16PM -0500, Qing Zhao 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)?
>> 
>> 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? 
> 
> Yes.
> 
>>> 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?
> 
> C requires that int is at least 16-bit wide, so the sizeof (int) == sizeof
> (char) case is only possible say with 16-bit char and 16-bit int, or 32-bit
> char and 32-bit int etc.
> 
>>> 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?
> 
> No.  Consider p[n] being e.g. 1 and q[n] being __SCHAR_MAX__ + 3U and 16-bit
> int and 16-bit char.  Then (unsigned char) 0x0001 < (unsigned char) 0x8002,
> so it should return a negative number.  But (int) (0x0001U - 0x8002U) is
> 0x7fff, which is a positive int.  Now, if int is 17-bit and char is 16-bit,
> this works fine, because is then -0x8001 and thus negative.

Okay, I see now.
really appreciate for your detailed explanation.
> 
> The above really works only if int is at least one bit wider than unsigned
> char.

Then, I will add a check to exclude the inlining when int is NOT wider than unsigned char on the target.

is the following the correct check:  (exp is the call to strcmp)

 if (CHAR_TYPE_SIZE >= TYPE_PRECISION (TREE_TYPE (exp)))

 
Thanks.

Qing

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

[Qing Zhao]

[-- Attachment #1: Type: text/plain, Size: 1182 bytes --]

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 #2: 78809C_uchar.patch --]
[-- Type: application/octet-stream, Size: 6166 bytes --]

From 1e1c3c4dc9b357f76347178c08cf1061d2915c9f Mon Sep 17 00:00:00 2001
From: qing zhao <qing.zhao@oracle.com>
Date: Fri, 20 Jul 2018 07:45:02 -0700
Subject: [PATCH] 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. Change char to unsigned char for strcmp/strncmp when expand
 them to a sequence of byte comparisons.

Due to C standard 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.

bootstraped and tested on both X86 and Aarch64. no regression.
---
 gcc/builtins.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/gcc/builtins.c b/gcc/builtins.c
index c069d66..0e805c5 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -119,7 +119,7 @@ static rtx expand_builtin_next_arg (void);
 static rtx expand_builtin_va_start (tree);
 static rtx expand_builtin_va_end (tree);
 static rtx expand_builtin_va_copy (tree);
-static rtx inline_expand_builtin_string_cmp (tree, rtx, bool);
+static rtx inline_expand_builtin_string_cmp (tree, rtx);
 static rtx expand_builtin_strcmp (tree, rtx);
 static rtx expand_builtin_strncmp (tree, rtx, machine_mode);
 static rtx builtin_memcpy_read_str (void *, HOST_WIDE_INT, scalar_int_mode);
@@ -4472,7 +4472,7 @@ expand_builtin_memcmp (tree exp, rtx target, bool result_eq)
 
   if (!result_eq && fcode != BUILT_IN_BCMP && no_overflow)
     {
-      result = inline_expand_builtin_string_cmp (exp, target, true);
+      result = inline_expand_builtin_string_cmp (exp, target);
       if (result)
 	return result;
     }
@@ -4551,7 +4551,7 @@ expand_builtin_strcmp (tree exp, ATTRIBUTE_UNUSED rtx target)
 
   /* Due to the performance benefit, always inline the calls first.  */
   rtx result = NULL_RTX;
-  result = inline_expand_builtin_string_cmp (exp, target, false);
+  result = inline_expand_builtin_string_cmp (exp, target);
   if (result)
     return result;
 
@@ -4670,7 +4670,7 @@ expand_builtin_strncmp (tree exp, ATTRIBUTE_UNUSED rtx target,
 
   /* Due to the performance benefit, always inline the calls first.  */
   rtx result = NULL_RTX;
-  result = inline_expand_builtin_string_cmp (exp, target, false);
+  result = inline_expand_builtin_string_cmp (exp, target);
   if (result)
     return result;
 
@@ -6764,22 +6764,24 @@ expand_builtin_goacc_parlevel_id_size (tree exp, rtx target, int ignore)
   
    to: (assume const_str_n is 2, i.e., arg2 is a constant string)
 
-   target = var_str[0] - const_str[0];
+   target = (int) (unsigned char) var_str[0]
+	    - (int) (unsigned char) const_str[0];
    if (target != 0)
      goto ne_label;
      ...
-   target = var_str[length - 2] - const_str[length - 2];
+   target = (int) (unsigned char) var_str[length - 2]
+	    - (int) (unsigned char) const_str[length - 2];
    if (target != 0)
      goto ne_label;
-   target = var_str[length - 1] - const_str[length - 1];
+   target = (int) (unsigned char) var_str[length - 1]
+	    - (int) (unsigned char) const_str[length - 1];
    ne_label:
   */
 
 static rtx
 inline_string_cmp (rtx target, tree var_str, const char *const_str,
 		   unsigned HOST_WIDE_INT length,
-		   int const_str_n, machine_mode mode,
-		   bool is_memcmp)
+		   int const_str_n, machine_mode mode)
 {
   HOST_WIDE_INT offset = 0;
   rtx var_rtx_array
@@ -6788,7 +6790,7 @@ inline_string_cmp (rtx target, tree var_str, const char *const_str,
   rtx const_rtx = NULL_RTX;
   rtx result = target ? target : gen_reg_rtx (mode);
   rtx_code_label *ne_label = gen_label_rtx ();
-  tree unit_type_node = is_memcmp ? unsigned_char_type_node : char_type_node;
+  tree unit_type_node = unsigned_char_type_node;
   scalar_int_mode unit_mode
     = as_a <scalar_int_mode> TYPE_MODE (unit_type_node);
 
@@ -6802,8 +6804,10 @@ inline_string_cmp (rtx target, tree var_str, const char *const_str,
       rtx op0 = (const_str_n == 1) ? const_rtx : var_rtx;
       rtx op1 = (const_str_n == 1) ? var_rtx : const_rtx;
 
+      op0 = convert_modes (mode, unit_mode, op0, 1);
+      op1 = convert_modes (mode, unit_mode, op1, 1);
       result = expand_simple_binop (mode, MINUS, op0, op1,
-				    result, is_memcmp ? 1 : 0, OPTAB_WIDEN);
+				    result, 1, OPTAB_WIDEN);
       if (i < length - 1)
 	emit_cmp_and_jump_insns (result, CONST0_RTX (mode), NE, NULL_RTX,
 	    			 mode, true, ne_label);
@@ -6822,7 +6826,7 @@ inline_string_cmp (rtx target, tree var_str, const char *const_str,
    TARGET if that's convenient.
    If the call is not been inlined, return NULL_RTX.  */
 static rtx
-inline_expand_builtin_string_cmp (tree exp, rtx target, bool is_memcmp)
+inline_expand_builtin_string_cmp (tree exp, rtx target)
 {
   tree fndecl = get_callee_fndecl (exp);
   enum built_in_function fcode = DECL_FUNCTION_CODE (fndecl);
@@ -6833,6 +6837,12 @@ inline_expand_builtin_string_cmp (tree exp, rtx target, bool is_memcmp)
 		       || fcode == BUILT_IN_STRNCMP
 		       || fcode == BUILT_IN_MEMCMP);
 
+  /* On a target where the type of the call (int) has same of narrower presicion
+     than unsigned char, give up the inlining expansion.  */
+  if (TYPE_PRECISION (unsigned_char_type_node)
+      >= TYPE_PRECISION (TREE_TYPE (exp)))
+    return NULL_RTX;
+
   tree arg1 = CALL_EXPR_ARG (exp, 0);
   tree arg2 = CALL_EXPR_ARG (exp, 1);
   tree len3_tree = is_ncmp ? CALL_EXPR_ARG (exp, 2) : NULL_TREE;
@@ -6879,7 +6889,7 @@ inline_expand_builtin_string_cmp (tree exp, rtx target, bool is_memcmp)
   /* Now, start inline expansion the call.  */
   return inline_string_cmp (target, (const_str_n == 1) ? arg2 : arg1,
 			    (const_str_n == 1) ? src_str1 : src_str2, length,
-			    const_str_n, mode, is_memcmp);
+			    const_str_n, mode);
 }
 
 /* Expand an expression EXP that calls a built-in function,
-- 
1.9.1

[-- Attachment #3: Type: text/plain, Size: 1788 bytes --]



> 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.

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

[Jakub Jelinek]

On Fri, Jul 20, 2018 at 09:53:24AM -0500, Qing Zhao wrote:
> +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.

s/presicion/precision/

Also in the patch, where there is another typo, s/of/or/.

Ok for trunk with that fixed.

	Jakub

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

[Qing Zhao]

> On Jul 20, 2018, at 9:59 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> On Fri, Jul 20, 2018 at 09:53:24AM -0500, Qing Zhao wrote:
>> +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.
> 
> s/presicion/precision/
> 
> Also in the patch, where there is another typo, s/of/or/.

Okay.
> 
> Ok for trunk with that fixed.

thanks a lot for the review.

Qing
> 
> 	Jakub

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

[Qing Zhao]

the patch was committed as:

https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=262907 <https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=262907>

thanks.

Qing
> On Jul 20, 2018, at 9:59 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> On Fri, Jul 20, 2018 at 09:53:24AM -0500, Qing Zhao wrote:
>> +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.
> 
> s/presicion/precision/
> 
> Also in the patch, where there is another typo, s/of/or/.
> 
> Ok for trunk with that fixed.
> 
> 	Jakub