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/09/2018 01:28 PM, Qing Zhao wrote:
Hi, Martin,

thanks a lot for your comments.

On Jul 5, 2018, at 11:30 AM, Martin Sebor <> wrote:

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.

I agree that warning options should not impact the emitted object code.
and my current change seems violate this principle:

in the following change:

+  bool no_overflow_warn = true;

   /* Diagnose calls where the specified length exceeds the size of either
      object.  */
   if (warn_stringop_overflow)
       tree size = compute_objsize (arg1, 0);
-      if (check_access (exp, /*dst=*/NULL_TREE, /*src=*/NULL_TREE, len,
-                       /*maxread=*/NULL_TREE, size, /*objsize=*/NULL_TREE))
+      no_overflow_warn = check_access (exp, /*dst=*/NULL_TREE, /*src=*/NULL_TREE,
+                                      len, /*maxread=*/NULL_TREE, size,
+                                      /*objsize=*/NULL_TREE);
+      if (no_overflow_warn)
          size = compute_objsize (arg2, 0);
-         check_access (exp, /*dst=*/NULL_TREE, /*src=*/NULL_TREE, len,
-                       /*maxread=*/NULL_TREE, size, /*objsize=*/NULL_TREE);
+         no_overflow_warn = check_access (exp, /*dst=*/NULL_TREE, /*src=*/NULL_TREE,
+                                          len,  /*maxread=*/NULL_TREE, size,
+                                          /*objsize=*/NULL_TREE);

+  /* Due to the performance benefit, always inline the calls first
+     when result_eq is false.  */
+  rtx result = NULL_RTX;
+  if (!result_eq && fcode != BUILT_IN_BCMP && no_overflow_warn)
+    {
+      result = inline_expand_builtin_string_cmp (exp, target, true);
+      if (result)

The variable no_overflow_warn DEPENDs on the warning option warn_stringop_overflow, and this
variable is used to control the code generation.  such behavior seems violate the above mentioned

However, this is not a problem that can be easily fixed based on the the current design, which has the following issues as my

	1. the routine check_access issues warnings by default, then it seems necessary to guard the call
to this routine with the warning option;
	2. then the returned value of the routine check_access has to depend on the warning option.

in order to fix the current problem I have, an approach is to rewrite the routine check_access to guard the issue warning inside
the routine with the warning option passed as an additional parameter.

let me know anything I am missing so far.

check_access() calls warning_at() to issue warnings, and that
function only issues warnings if they are enabled, so the guard
isn't necessary to make it work this way.

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.

Yes, I agree that this will be another good enhancement to the strcmp inlining.

however, it’s not easy to be integrated with my current patch.  The major issue is:

	 The inlined code for the strcmp call without string constant will be different than the inlined code for the
strcmp call with string constant,  then:

	1. the default value for the threshold that control the maximum length of the string length for inlining will
be different than the one for the strcmp call with string constant,  more experiments need to be run and a new parameter
need to be added to control this;
	2. the inlined transformed code will be different than the current one.

based on the above, I’d like to open a new PR to record this new enhancement and finish it with a new patch later.

what’s your opinion on this?

I'm not sure I see the issues above as problems and I would expect
the non-constant optimization to naturally handle the constant case
as well.  But if you prefer it that way, implementing the non-constant
optimization in a separate step sounds reasonable to me.  It's your


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