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

[Jonathan Wakely <jwakely at redhat dot com>]

On 11/12/17 12:35 +0000, Jonathan Wakely wrote:
> On 10/12/17 22:13 +0100, François Dumont wrote:
> > On 06/12/2017 04:44, Martin Sebor wrote:
> > > 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())
> >
> > Or perhaps do something like that:
> >
> > @@ -485,7 +485,9 @@ namespace std
> >  #if defined(_GLIBCXX_ASSERTIONS)
> >  # define __glibcxx_assert(_Condition) __glibcxx_assert_impl(_Condition)
> >  #else
> > -# define __glibcxx_assert(_Condition)
> > +# define __glibcxx_assert(_Condition)                                  \
> > +    ((_Condition) ? (void)0 : __builtin_unreachable())
> >  #endif
> >
> > So that assertions are automatically transform into optimizer hints 
> > on release builds.
>
> That's an interesting idea, but quite scary to make that change at
> this point in Stage 3, as it would affect dozens of places in the
> code. We should consider it in Stage 1 though.
>
> This might be a slightly better way to do it:
>
> --- a/libstdc++-v3/include/bits/c++config
> +++ b/libstdc++-v3/include/bits/c++config
> @@ -458,6 +458,9 @@ namespace std
>
> #if defined(_GLIBCXX_ASSERTIONS)
> # define __glibcxx_assert(_Condition) __glibcxx_assert_impl(_Condition)
> +#elif defined(__OPTIMIZE__)
> +# define __glibcxx_assert(_Condition) \
> +  do { if (! (_Condition)) __builtin_unreachable(); } while (false)
> #else
> # define __glibcxx_assert(_Condition)
> #endif
>
> Since the __builtin_unreachable won't be used when not optimizing, so
> there's no point evaluating the condition.
>
> All our tests pass with this change.

Thinking about it further, there are places we'd want to say "assume"
but don't want to "assert" (e.g. where Martin's patch adds
__builtin_unreachable) because we want to tell the compiler about
invariants, not just check user input. We wouldn't want to use
__glibcxx_assert for the change Martin is making.

So it would be better to have the __glibcxx_assume macro for those
situations, and then make __glibcxx_assert use __glibcxx_assume when
assertions are disabled.