[PATCH][Middle-end][version 2]2nd patch of PR78809 and PR83026

[Richard Biener]

On Fri, 12 Jan 2018, Jeff Law wrote:

> On 12/21/2017 02:25 PM, Qing Zhao wrote:
> > Hi, 
> > 
> > I updated my patch based on all your comments. 
> > 
> > the major changes are the following:
> > 
> > 	1. replace the candidate calls with __builtin_str(n)cmp_eq instead of __builtin_memcmp_eq;
> >             in builtins.c,  when expanding the new __builtin_str(n)cmp_eq call, expand them first as
> >             __builtin_memcmp_eq, if Not succeed,  change the call back to __builtin_str(n)cmp.
> > 	2. change the call to “get_range_strlen” with “compute_objsize”.
> Please read the big comment before compute_objsize.  If you are going to
> use it to influence code generation or optimization, then you're most
> likely doing something wrong.
> 
> compute_objsize can return an estimate in some circumstances.
> 
> 
> > 	3. add the missing case for equality checking with zero;
> > 	4. adjust the new testing case for PR83026; add a new testing case for the missing case added in 3.
> > 	5. update “uhwi” to “shwi” for where it needs;
> > 	6. some other minor format changes.
> > 
> > the changes are retested on x86 and aarch64, bootstrapped and regression tested. no issue.
> > 
> > Okay for trunk?
> > 
> > thanks.
> > 
> > Qing
> > 
> > Please see the updated patch:
> > 
> > gcc/ChangeLog:
> > 
> > +2017-12-21  Qing Zhao  <qing.zhao@oracle.com>
> > +
> > +	PR middle-end/78809
> > +	PR middle-end/83026
> > +	* builtins.c (expand_builtin): Add the handling of BUILT_IN_STRCMP_EQ
> > +	and BUILT_IN_STRNCMP_EQ.
> > +	* builtins.def: Add new builtins BUILT_IN_STRCMP_EQ and
> > +	BUILT_IN_STRNCMP_EQ.
> > +	* tree-ssa-strlen.c (compute_string_length): New function.
> > +	(handle_builtin_string_cmp): New function to handle calls to
> > +	string compare functions.
> > +	(strlen_optimize_stmt): Add handling to builtin string compare
> > +	calls. 
> > +	* tree.c (build_common_builtin_nodes): Add new defines of
> > +	BUILT_IN_STRNCMP_EQ and BUILT_IN_STRCMP_EQ.
> > +
> > gcc/testsuite/ChangeLog
> > 
> > +2017-12-21  Qing Zhao  <qing.zhao@oracle.com> 
> > +
> > +	PR middle-end/78809
> > +	* gcc.dg/strcmpopt_2.c: New testcase.
> > +	* gcc.dg/strcmpopt_3.c: New testcase.
> > +
> > +	PR middle-end/83026
> > +	* gcc.dg/strcmpopt_3.c: New testcase.
> What I don't like here is the introduction of STRCMP_EQ and STRNCMP_EQ.
> ISTM that if you're going to introduce those new builtins, then you have
> to audit all the code that runs between their introduction into the IL
> and when you expand them to ensure they're handled properly.
> 
> All you're really doing is carrying along a status bit about what
> tree-ssa-strlen did.  So you could possibly store that status bit somewhere.
> 
> The concern with both is that something later invalidates the analysis
> you've done.  I'm having a hard time coming up with a case where this
> could happen, but I'm definitely concerned about this possibility.
> Though I feel it's more likely to happen if we store a status bit vs
> using STRCMP_EQ STRNCMP_EQ.
> 
> [ For example, we have two calls with the same arguments, but one feeds
> an equality test, the other does not.  If we store a status bit that one
> could be transformed, but then we end up CSE-ing the two calls, the
> status bit would be incorrect because one of the calls did not feed an
> equality test.  Hmmm. ]
> 
> 
> 
> 
> > diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
> > index 94f20ef..57563ef 100644
> > --- a/gcc/tree-ssa-strlen.c
> > +++ b/gcc/tree-ssa-strlen.c
> > @@ -2540,6 +2540,216 @@ handle_builtin_memcmp (gimple_stmt_iterator *gsi)
> >    return false;
> >  }
> >  
> > +/* Given an index to the strinfo vector, compute the string length for the
> > +   corresponding string. Return -1 when unknown.  */
> > + 
> > +static HOST_WIDE_INT 
> > +compute_string_length (int idx)
> > +{
> > +  HOST_WIDE_INT string_leni = -1; 
> > +  gcc_assert (idx != 0);
> > +
> > +  if (idx < 0)
> > +    string_leni = ~idx;
> So it seems to me you should just
>   return ~idx;
> 
> Then appropriately simplify the rest of the code.
> 
> > +
> > +/* Handle a call to strcmp or strncmp. When the result is ONLY used to do 
> > +   equality test against zero:
> > +
> > +   A. When both arguments are constant strings and it's a strcmp:
> > +      * if the length of the strings are NOT equal, we can safely fold the call
> > +        to a non-zero value.
> > +      * otherwise, do nothing now.
> I'm guessing your comment needs a bit of work.  If both arguments are
> constant strings, then we can just use the host str[n]cmp to fold the
> str[n]cmp to a constant.  Presumably this is handled earlier :-)
> 
> So what I'm guessing is you're really referring to the case where the
> lengths are known constants, even if the contents of the strings
> themselves are not.  In that case if its an equality comparison, then
> you can fold to a constant.  Otherwise we do nothing.  So I think the
> comment needs updating here.
> 
> 
> 
> 
> 
> 
> > +  
> > +   B. When one of the arguments is constant string, try to replace the call with
> > +   a __builtin_str(n)cmp_eq call where possible, i.e:
> > +
> > +   strncmp (s, STR, C) (!)= 0 in which, s is a pointer to a string, STR is a 
> > +   constant string, C is a constant.
> > +     if (C <= strlen(STR) && sizeof_array(s) > C)
> > +       {
> > +         replace this call with
> > +         strncmp_eq (s, STR, C) (!)= 0
> > +       }
> > +     if (C > strlen(STR)
> > +       {
> > +         it can be safely treated as a call to strcmp (s, STR) (!)= 0
> > +         can handled by the following strcmp.
> > +       }
> > +
> > +   strcmp (s, STR) (!)= 0 in which, s is a pointer to a string, STR is a 
> > +   constant string.
> > +     if  (sizeof_array(s) > strlen(STR))
> > +       {
> > +         replace this call with
> > +         strcmp_eq (s, STR, strlen(STR)+1) (!)= 0
> So I'd hazard a guess that this comment has the same problem.  It's
> mixing up the concept of a constant string and the concept of a
> nonconstant string, but which has a known constant length.
> 
> First, if one of the arguments is a string constant, then it should be
> easy to get the constant at expansion time with minimal effort.  It's
> also the case that knowing if we're comparing the result against zero is
> trivial to figure out at expansion time.  This would probably be a
> reasonble thing to add to the expanders.
> 
> 
> 
> Your function comment for handle_builtin_string_cmp does not indicate
> what the return value means.  From reading the code is appears to return
> TRUE when it does nothing and FALSE when it optimizes the call in some
> way.  Is that correct?  That seems inverted from the way we'd normally
> write this stuff.
> 
> 
> 
> > +
> > +  /* When one of args is constant string.  */
> > +  tree var_string = NULL_TREE;
> > +  HOST_WIDE_INT const_string_leni = -1;
> > +  
> > +  if (idx1)
> > +    {
> > +      const_string_leni = compute_string_length (idx1);
> > +      var_string = arg2;
> > +    } 
> > +  else 
> > +    {
> > +      gcc_checking_assert (idx2);
> > +      const_string_leni = compute_string_length (idx2);
> > +      var_string = arg1;
> > +    } 
> > +
> > +  if (const_string_leni < 0) 
> > +    return true;
> > + 
> > +  unsigned HOST_WIDE_INT var_sizei = 0;
> > +  /* try to determine the minimum size of the object pointed by var_string.  */
> > +  tree size = compute_objsize (var_string, 2);
> So this worries me as well.  compute_objsize is not supposed to be used
> to influence code generation or optimization.  It is not guaranteed to
> return an exact size, but instead may return an estimate!

Yes, compute_objsize cannot be used for optimizations.

> I'd really like to hear Jakub or Richi chime in with their thoughts on
> how this transforms one builtin into another and whether or not they
> think that is wise.

Given it follows how we handle memcmp () != 0 it's ok to trigger
inline expansion.  Didn't look into the patch whether it does this.
And yes, places that look at STR[N]CMP now also have to look at
STR[N]CMP_EQ.

We're now in stage4 so please queue this patch and ping it during
next stage1.

Thanks,
Richard.