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

[Qing Zhao <qing dot zhao at oracle dot com>]

Richard and Martin,

thanks for the info.

> On Jul 10, 2018, at 11:29 AM, Richard Biener <rguenther@suse.de> wrote:
>> Is the above condition on variable warn_stringop_overflow unnecessary?
>> all the warnings inside check_access are controlled by
>> OPT_Wstringop_overflow_.
> 
> Well, the condition certainly saves compile time. 



> On Jul 10, 2018, at 11:55 AM, Martin Sebor <msebor@gmail.com> wrote:
>> 
>> Is the above condition on variable warn_stringop_overflow unnecessary?
>> all the warnings inside check_access are controlled by OPT_Wstringop_overflow_.
>> 
>> can I safely delete the above condition if (warn_stringop_overflow)?
> 
> I think the check above is only there to avoid the overhead
> of the two calls to compute_objsize and check_access.  There
> are a few more like it in other functions in the file and
> they all should be safe to remove, but also safe to keep.
> (Some of them might make it easy to inadvertently introduce
> a dependency between the warning option and an optimization
> so that's something to consider.)

currently,  the condition is there for saving compilation time.
However, for my patch, I need the return value of check_access to control whether 
to invoking inlining or not,  therefore,  the call to check_access should always be
invoked for code generation.  The condition need to be deleted.

let me know if I still miss anything here.

>>>> 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.
>> 
>> the inlined code for call to strcmp with constant string will only have one load instruction for each byte, but for call to strcmp
>> without constant string, there will be  two load instructions for each byte.  So, the run time performance impact will be different.
>> we need separate default values of the maximum length of the string to enable the transformation.
> 
> You're right, that's true for builtins.c where all we have to
> work with is arrays with unknown contents and string literals.
> The strlen pass, on the other hand, has access to the lengths
> of even unknown strings.  That suggests that an even better
> place for the optimization might be the strlen pass where
> the folding could happen earlier and at a higher level, which
> might even obviate having to worry about the constant vs non-
> constant handling.

Yes, looks like the inlining of call to strcmp with all variable strings might need to be done at
strlen pass in order to get more necessary info. 

In addition to this, I still feel that these two inlining could be separated.  the generated code of inlining of call to strcmp with constant string
could be more optimal than the inlining of call to strcmp without constant strings. the cost models are different.

I just created PR:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86467 <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86467>

for this work.

> 
>> I will create a PR on this and add a new patch after this one.
> 
> Sure, one step at a time makes sense.  I don't think there is
> any harm in having the optimization in two places: builtins.c
> and strlen.

Thanks a lot for your suggestions.

Qing