[RFC PATCH] expand_strn_compare should attempt expansion even if neither string is constant

[RFC PATCH] expand_strn_compare should attempt expansion even if neither string is constant

[Aaron Sawdey]

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

I'm currently working on a builtin expansion of strncmp for powerpc
similar to the one for memcmp I checked recently. One thing I
encountered is that the code in expand_strn_compare will not attempt to
expand the cmpstrnsi pattern at all if neither string parameter is a
constant string. This doesn't make a lot of sense in light of the fact
that expand_str_compare starts with an attempt to expand cmpstrsi and
then if that does not work, looks at the string args to see if one is
constant so it can use cmpstrnsi with the known length.

The attached patch is my attempt to add this fallback path to
expand_strn_compare, i.e. if neither length is known, just attempt
expansion of cmpstrnsi using the given 3 arguments.

It looks like in addition to rs6000, there are 3 other targets that
have cmpstrnsi patterns: i386, sh, and rx. 

Is this a reasonable approach to take with this? If so I'll
bootstrap/regtest on i386 as rs6000 does not as yet have an expansion
for cmpstrsi or cmpstrnsi.

Thanks,
   Aaron

-- 
Aaron Sawdey, Ph.D.  acsawdey@linux.vnet.ibm.com
050-2/C113  (507) 253-7520 home: 507/263-0782
IBM Linux Technology Center - PPC Toolchain

[-- Attachment #2: expand_strn.patch --]
[-- Type: text/x-patch, Size: 3200 bytes --]

Index: builtins.c
===================================================================
--- builtins.c	(revision 241593)
+++ builtins.c	(working copy)
@@ -3932,46 +3932,53 @@
     len1 = c_strlen (arg1, 1);
     len2 = c_strlen (arg2, 1);
 
-    if (len1)
-      len1 = size_binop_loc (loc, PLUS_EXPR, ssize_int (1), len1);
-    if (len2)
-      len2 = size_binop_loc (loc, PLUS_EXPR, ssize_int (1), len2);
+    if (!len1 && !len2)
+      {
+	len = arg3;
+      }
+    else
+      {
+	if (len1)
+	  len1 = size_binop_loc (loc, PLUS_EXPR, ssize_int (1), len1);
+	if (len2)
+	  len2 = size_binop_loc (loc, PLUS_EXPR, ssize_int (1), len2);
 
-    /* If we don't have a constant length for the first, use the length
-       of the second, if we know it.  We don't require a constant for
-       this case; some cost analysis could be done if both are available
-       but neither is constant.  For now, assume they're equally cheap,
-       unless one has side effects.  If both strings have constant lengths,
-       use the smaller.  */
+	/* If we don't have a constant length for the first, use the length
+	   of the second, if we know it.  We don't require a constant for
+	   this case; some cost analysis could be done if both are available
+	   but neither is constant.  For now, assume they're equally cheap,
+	   unless one has side effects.  If both strings have constant lengths,
+	   use the smaller.  */
 
-    if (!len1)
-      len = len2;
-    else if (!len2)
-      len = len1;
-    else if (TREE_SIDE_EFFECTS (len1))
-      len = len2;
-    else if (TREE_SIDE_EFFECTS (len2))
-      len = len1;
-    else if (TREE_CODE (len1) != INTEGER_CST)
-      len = len2;
-    else if (TREE_CODE (len2) != INTEGER_CST)
-      len = len1;
-    else if (tree_int_cst_lt (len1, len2))
-      len = len1;
-    else
-      len = len2;
+	if (!len1)
+	  len = len2;
+	else if (!len2)
+	  len = len1;
+	else if (TREE_SIDE_EFFECTS (len1))
+	  len = len2;
+	else if (TREE_SIDE_EFFECTS (len2))
+	  len = len1;
+	else if (TREE_CODE (len1) != INTEGER_CST)
+	  len = len2;
+	else if (TREE_CODE (len2) != INTEGER_CST)
+	  len = len1;
+	else if (tree_int_cst_lt (len1, len2))
+	  len = len1;
+	else
+	  len = len2;
 
-    /* If both arguments have side effects, we cannot optimize.  */
-    if (!len || TREE_SIDE_EFFECTS (len))
-      return NULL_RTX;
+	/* If both arguments have side effects, we cannot optimize.  */
+	if (!len || TREE_SIDE_EFFECTS (len))
+	  return NULL_RTX;
 
-    /* The actual new length parameter is MIN(len,arg3).  */
-    len = fold_build2_loc (loc, MIN_EXPR, TREE_TYPE (len), len,
-		       fold_convert_loc (loc, TREE_TYPE (len), arg3));
+	/* The actual new length parameter is MIN(len,arg3).  */
+	len = fold_build2_loc (loc, MIN_EXPR, TREE_TYPE (len), len,
+			       fold_convert_loc (loc, TREE_TYPE (len), arg3));
 
-    /* If we don't have POINTER_TYPE, call the function.  */
-    if (arg1_align == 0 || arg2_align == 0)
-      return NULL_RTX;
+	/* If we don't have POINTER_TYPE, call the function.  */
+	if (arg1_align == 0 || arg2_align == 0)
+	  return NULL_RTX;
+      }
 
     /* Stabilize the arguments in case gen_cmpstrnsi fails.  */
     arg1 = builtin_save_expr (arg1);

Re: [RFC PATCH] expand_strn_compare should attempt expansion even if neither string is constant

[Bernd Schmidt]

On 10/27/2016 03:14 AM, Aaron Sawdey wrote:
> I'm currently working on a builtin expansion of strncmp for powerpc
> similar to the one for memcmp I checked recently. One thing I
> encountered is that the code in expand_strn_compare will not attempt to
> expand the cmpstrnsi pattern at all if neither string parameter is a
> constant string. This doesn't make a lot of sense in light of the fact
> that expand_str_compare starts with an attempt to expand cmpstrsi and
> then if that does not work, looks at the string args to see if one is
> constant so it can use cmpstrnsi with the known length.
>
> The attached patch is my attempt to add this fallback path to
> expand_strn_compare, i.e. if neither length is known, just attempt
> expansion of cmpstrnsi using the given 3 arguments.
>
> It looks like in addition to rs6000, there are 3 other targets that
> have cmpstrnsi patterns: i386, sh, and rx.
>
> Is this a reasonable approach to take with this? If so I'll
> bootstrap/regtest on i386 as rs6000 does not as yet have an expansion
> for cmpstrsi or cmpstrnsi.

Just to be sure, this is superseded by your later patch series, correct?

(After I saw this one I had been trying to remember what exactly the 
i386 issue was but it looks like you found it yourself.)


Bernd

Re: [RFC PATCH] expand_strn_compare should attempt expansion even if neither string is constant

[Aaron Sawdey]

On Wed, 2016-11-02 at 13:41 +0100, Bernd Schmidt wrote:
> On 10/27/2016 03:14 AM, Aaron Sawdey wrote:
> > 
> > I'm currently working on a builtin expansion of strncmp for powerpc
> > similar to the one for memcmp I checked recently. One thing I
> > encountered is that the code in expand_strn_compare will not
> > attempt to
> > expand the cmpstrnsi pattern at all if neither string parameter is
> > a
> > constant string. This doesn't make a lot of sense in light of the
> > fact
> > that expand_str_compare starts with an attempt to expand cmpstrsi
> > and
> > then if that does not work, looks at the string args to see if one
> > is
> > constant so it can use cmpstrnsi with the known length.
> > 
> > The attached patch is my attempt to add this fallback path to
> > expand_strn_compare, i.e. if neither length is known, just attempt
> > expansion of cmpstrnsi using the given 3 arguments.
> > 
> > It looks like in addition to rs6000, there are 3 other targets that
> > have cmpstrnsi patterns: i386, sh, and rx.
> > 
> > Is this a reasonable approach to take with this? If so I'll
> > bootstrap/regtest on i386 as rs6000 does not as yet have an
> > expansion
> > for cmpstrsi or cmpstrnsi.
> 
> Just to be sure, this is superseded by your later patch series,
> correct?
> 
> (After I saw this one I had been trying to remember what exactly the 
> i386 issue was but it looks like you found it yourself.)
> 
> 
> Bernd

Yes, the later patch series replaces this preliminary patch. And yes,
the i386 issue took some headscratching and careful reading of the arch
manual on what repz cmpsb actually does.

Thanks,
   Aaron

-- 
Aaron Sawdey, Ph.D.  acsawdey@linux.vnet.ibm.com
050-2/C113  (507) 253-7520 home: 507/263-0782
IBM Linux Technology Center - PPC Toolchain