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

[Martin Sebor]

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 <msebor@gmail.com> 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
> principle.
>
> However, this is not a problem that can be easily fixed based on the the current design, which has the following issues as my
> understanding:
>
> 	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
call.

Martin