Re: [PATCH][Middle-end]3rd patch of PR78809

[Martin Sebor <msebor at gmail dot com>]

On 07/05/2018 09:46 AM, Qing Zhao wrote:
> Hi,
>
> I have sent two emails with the updated patches on 7/3:
>
> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg00065.html
> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg00070.html
>
> however, these 2 emails  were not successfully forwarded to the gcc-patches@gcc.gnu.org mailing list.
>
> So, I am sending the same email again in this one, hopefully this time it can go through.
> Qing

Thanks for taking care to issue the warnings (and thanks to
Jeff for pointing it out)!

One of the basic design principles that I myself have
accidentally violated in the past is that warning options
should not impact the emitted object code.  I don't think
your patch actually does introduce this dependency by having
the codegen depend on the result of check_access() -- I'm
pretty sure the function is designed to do the validation
irrespective of warning options and return based on
the result of the validation and not based on whether
a warning was issued.  But the choice of the variable name,
no_overflow_warn, suggests that it does, in fact, have this
effect.  So I would suggest to rename the variable and add
a test that verifies that this dependency does not exist.

Beyond that, an enhancement to this optimization that might
be worth considering is inlining even non-constant calls
with array arguments whose size is no greater than the limit.
As in:

  extern char a[4], *b;

  int n = strcmp (a, b);

Because strcmp arguments are required to be nul-terminated
strings, a's length above must be at most 3.  This is analogous
to similar optimizations GCC performs, such as folding to zero
calls to strlen() with one-element arrays.

Martin

> Hi, Jeff,
>
> thanks a lot for your review and comments.
>
> I have addressed your comments,updated the patch, retested on both
> aarch64 and x86.
>
> The major changes in this version compared to the previous version are:
>
> 	1. in routine expand_builtin_memcmp:
> * move the inlining transformation AFTER the warning is issues for
> -Wstringop-overflow;
> * only apply inlining when there is No warning is issued.
> 	2. in the testsuite, add a new testcase strcmpopt_6.c for this case.
> 	3. update comments to:
> * capitalize the first word.
> * capitalize all the arguments.
>
> NOTE, the routine expand_builtin_strcmp and expand_builtin_strncmp are not changed.
> the reason is:  there is NO overflow checking for these two routines currently.
> if we need overflow checking for these two routines, I think that a separate patch is needed.
> if this is needed, let me know, I can work on this separate patch for issuing warning for strcmp/strncmp when
> -Wstringop-overflow is specified.
>
> The new patch is as following, please take a look at it.
>
> thanks.
>
> Qing
>
> gcc/ChangeLog
>
> +2018-07-02  Qing Zhao  <qing.zhao@oracle.com>
> +
> +	PR middle-end/78809
> +	* builtins.c (expand_builtin_memcmp): Inline the calls first
> +	when result_eq is false.
> +	(expand_builtin_strcmp): Inline the calls first.
> +	(expand_builtin_strncmp): Likewise.
> +	(inline_string_cmp): New routine. Expand a string compare
> +	call by using a sequence of char comparison.
> +	(inline_expand_builtin_string_cmp): New routine. Inline expansion
> +	a call to str(n)cmp/memcmp.
> +	* doc/invoke.texi (--param builtin-string-cmp-inline-length): New option.
> +	* params.def (BUILTIN_STRING_CMP_INLINE_LENGTH): New.
> +
>
> gcc/testsuite/ChangeLog
>
> +2018-07-02  Qing Zhao  <qing.zhao@oracle.com>
> +
> +	PR middle-end/78809
> +	* gcc.dg/strcmpopt_5.c: New test.
> +	* gcc.dg/strcmpopt_6.c: New test.
> +
>
>
>
>
>
> > On Jun 28, 2018, at 12:10 AM, Jeff Law <law@redhat.com> wrote:
> >
> >
> > So this generally looks pretty good.  THe biggest technical concern is
> > making sure we're doing the right thing WRT issuing warnings.  You can
> > tackle that problem by deferring inlining to a later point after
> > warnings have been issued or by verifying that your routines do not
> > inline in cases where warnings will be issued.  It may be worth adding
> > testcases for these issues.
> >
> > There's a large number of comments that need capitalization fixes.
> >
> > Given there was no measured runtime performance impact, but slight
> > improvements on codesize for values <= 3, let's go ahead with that as
> > the default.
> >
> > Can you address the issues above and repost for final review?
> >
> > Thanks,
> > jeff