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