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

[Jeff Law]

On 06/28/2018 01:12 AM, Richard Biener wrote:
> On Wed, 27 Jun 2018, Jeff Law wrote:
> 
>>
>> On 06/18/2018 09:37 AM, Qing Zhao wrote:
>>> Hi,
>>>
>>> this is the 3rd (and the last) patch for PR78809:
>>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78809
>>> Inline strcmp with small constant strings
>>>
>>> The design doc for PR78809 is at:
>>> https://www.mail-archive.com/gcc@gcc.gnu.org/msg83822.html
>>>
>>> this patch is for the third part of change of PR78809.
>>>
>>> C. for strcmp (s1, s2), strncmp (s1, s2, n), and memcmp (s1, s2, n)
>>>    if the result is NOT used to do simple equality test against zero, one of
>>> "s1" or "s2" is a small constant string, n is a constant, and the Min value of
>>> the length of the constant string and "n" is smaller than a predefined
>>> threshold T,
>>>    inline the call by a byte-to-byte comparision sequence to avoid calling
>>> overhead.
>>>
>>> adding test case strcmpopt_5.c into gcc.dg for part C of PR78809.
>>>
>>> bootstraped and tested on both X86 and Aarch64. no regression.
>>>
>>> I have done some experiments to check the run-time performance impact 
>>> and code-size impact from such inlining with different threshold for the
>>> length of the constant string to inline, the data were recorded in the attachments of 
>>> PR78809:
>>>
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78809.
>>>
>>> and finally decide that the default value of the threshold is 3. 
>>> this value of the threshold can be adjusted by the new option:
>>>
>>> --param builtin-string-cmp-inline-length=
>>>
>>> The following is the patch.
>>>
>>> thanks.
>>>
>>> Qing
>>>
>>> gcc/ChangeLog:
>>>
>>> +2018-06-18  Qing Zhao  <qing.zhao@oracle.com>
>>> +
>>> +       PR middle-end/78809
>>> +       * builtins.c (expand_builtin_memcmp): Inline the calls first
>>> +       when result_eq is false.
>>> +       (expand_builtin_strcmp): Inline the calls first.
>>> +       (expand_builtin_strncmp): Likewise.
>>> +       (inline_string_cmp): New routine. Expand a string compare 
>>> +       call by using a sequence of char comparison.
>>> +       (inline_expand_builtin_string_cmp): New routine. Inline expansion
>>> +       a call to str(n)cmp/memcmp.
>>> +       * doc/invoke.texi (--param builtin-string-cmp-inline-length): New option.
>>> +       * params.def (BUILTIN_STRING_CMP_INLINE_LENGTH): New.
>>> +
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> +2018-06-18  Qing Zhao  <qing.zhao@oracle.com>
>>> +
>>> +       PR middle-end/78809
>>> +       * gcc.dg/strcmpopt_5.c: New test.
>> So I still need to dig into this patch.  But I wanted to raise an
>> potential issue and get yours and Martin's thoughts on it.
>>
>> Martin (and others) have been working hard to improve GCC's ability to
>> give good diagnostics for various problems with calls to mem* and str*
>> functions (buffer overflows, restrict issues, etc).
>>
>> One of the problems Martin has identified is early conversion of these
>> calls into inlined direct operations.  If those conversions happen prior
>> to the analysis for warnings we obviously can't issue any relevant warnings.
> 
> From the looks of the changelog this seems to be done at RTL expansion 
> time -- which also makes me question why targets do not provide
> cmpstr[n] / cmpmem optabs when doing the inlining is so obviously
> beneficial?
> 
> Note I didn't look at the actual patch.
Some do.  And there's other code that does generic inlining of this
stuff (all the move by pieces related thingies).  Qing's patch is
handling another case where we have a bit more data on how the result it
used.

We could pass that info down to the targets, but then we'd have to
twiddle each one which would be painful.  In general I'd rather handle
most of the inlining decisions in the generic parts of the expanders
rather than in the targets themselves.

Jeff