Hi, Martin,
thanks a lot for your comments.
> On Jul 5, 2018, at 11:30 AM, Martin Sebor <mse...@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.
>
> 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?
Qing
>
> Martin
