[PATCH] annotate vector::_M_default_append fo better codegen (PR 83229)

[Marc Glisse]

On Tue, 5 Dec 2017, Martin Sebor wrote:

> Bug 83239 - False positive from -Wstringop-overflow on simple
> std::vector code, besides pointing out the warning, suggests
> a missed optimization opportunity.  This is the second report
> involving for vector (the last one was pr79095) with the same
> symptoms and a similar root cause.  Since the information GCC
> keeps about pointers is limited, it's difficult to determine
> that their relationship is p <= q <= r.  This is exacerbated
> by the fact that GCC doesn't know that the difference between
> any two pointers into the same object cannot be greater than
> PTRDIFF_MAX (bug 79119).
>
> To help GCC generate better code and avoid the false positive,
> the attached patch adds simple instrumentation to
> vector::_M_default_append() asserting the important pointer
> relationships.
>
> Beside the invariant in the patch, I tested a few other forms,
> including:
>
>  if (__size > max_size())
>    __builtin_unreachable ();

Did you consider adding the above assumption inside the size() function, 
for wider exposure? I am not sure what good benchmark exists with 
std::vector to see if on average that helps or hurts.

> and
>
>  if (__size > max_size() - size())
>    __builtin_unreachable ();
>
> Although the first one does the job, it's not quite as accurate
> as the one in the patch.  The second one above is, and should be
> sufficient, but isn't and actually turns out to be detrimental
> (triggers even more false positives and causes worse code for
> due to the same limitations).
>
> The form I used in the patch is effective, leads to better code,
> and avoids the false positive.  The code improvement I measured
> on x86_64 is in mostly to code size (10 instructions for the test
> case in the report), resulting from eliminating unreachable code.
> The speedup was in the noise.
>
> Since it's possible that there may be other opportunities for
> similar annotation in libstdc++, if changes along these lines
> become more than isolated instances, it may be worth considering
> adding a macro to make the conditions more readable.  E.g., to
> follow the example of __glibcxx_assert, something like:
>
>  #define __glibcxx_assume(expr) \
>    ((expr) ? (void)0 : __builtin_unreachable())
>
> Martin
>

-- 
Marc Glisse