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

[Qing Zhao <qing dot zhao at oracle dot com>]

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

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

Attachment: 0001-3nd-Patch-for-PR78009.patch
Description: Binary data

> 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