This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

On 07/05/2018 09:46 AM, Qing Zhao wrote:

I have sent two emails with the updated patches on 7/3:

however, these 2 emails  were not successfully forwarded to the mailing list.

So, I am sending the same email again in this one, hopefully this time it can go through.

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.


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




+2018-07-02  Qing Zhao  <>
+	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.


+2018-07-02  Qing Zhao  <>
+	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 <> 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?


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]