Add builtin likely to assertions

  The true part of assertions are more likely to get executed so adding likely attribute
to the true part will help with better basic block layout.

```
commit fe153e42a57550f9db667b130e3d00a178c27199 (HEAD -> master)
Author: AK <1894981+hiraditya@users.noreply.github.com>
Date:   Tue Jul 28 15:59:08 2020 -0700

    Add builtin likely to assertions


```

Thanks
-Aditya
  

On Tue, Jul 28, 2020 at 7:10 PM Aditya K via Libc-alpha
<libc-alpha@sourceware.org> wrote:
> The true part of assertions are more likely to get executed so adding likely attribute
> to the true part will help with better basic block layout.

Do you actually observe code layout changes with this patch, and if
so, with which compiler? This should already be handled by the
heuristic that any code path leading to a noreturn function is
unlikely.

zw
  

On Tue, Jul 28, 2020 at 4:10 PM Aditya K via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> The true part of assertions are more likely to get executed so adding likely attribute
> to the true part will help with better basic block layout.

This is not needed for GCC to do the correct thing as __assert_fail is
marked as noreturn and GCC heuristics for branch prediction already
predicts it correctly and has done since at least 4.4 (or even
beforehand).
Can you explain more on which compiler this helps with?

Thanks,
Andrew Pinski

>
> ```
> commit fe153e42a57550f9db667b130e3d00a178c27199 (HEAD -> master)
> Author: AK <1894981+hiraditya@users.noreply.github.com>
> Date:   Tue Jul 28 15:59:08 2020 -0700
>
>     Add builtin likely to assertions
>
> diff --git a/assert/assert.h b/assert/assert.h
> index 266ee92e..2933ff60 100644
> --- a/assert/assert.h
> +++ b/assert/assert.h
> @@ -87,7 +87,7 @@ __END_DECLS
>     suppress warnings we'd expect to be detected by gcc's -Wparentheses.  */
>  # if defined __cplusplus
>  #  define assert(expr)                                                 \
> -     (static_cast <bool> (expr)                                                \
> +     (__builtin_expect(static_cast <bool> (expr), 1)                   \
>        ? void (0)                                                       \
>        : __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION))
>  # elif !defined __GNUC__ || defined __STRICT_ANSI__
> @@ -103,7 +103,7 @@ __END_DECLS
>     suppress the evaluation of variable length arrays.  */
>  #  define assert(expr)                                                 \
>    ((void) sizeof ((expr) ? 1 : 0), __extension__ ({                    \
> -      if (expr)                                                                \
> +      if (__builtin_expect(expr, 1))                                   \
>          ; /* empty */                                                  \
>        else                                                             \
>          __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION);  \
>
> ```
>
> Thanks
> -Aditya
  

> This is not needed for GCC to do the correct thing as __assert_fail is
> marked as noreturn and GCC heuristics for branch prediction already
> predicts it correctly and has done since at least 4.4 (or even
> beforehand).
> Can you explain more on which compiler this helps with?

I saw this pattern in 'MacOSX.sdk/usr/include/assert.h', so added here. I don't have any test case to show a different layout with this patch.

-Aditya
  

On 7/29/20 2:05 AM, Andrew Pinski via Libc-alpha wrote:
> This is not needed for GCC to do the correct thing as __assert_fail is
> marked as noreturn and GCC heuristics for branch prediction already
> predicts it correctly and has done since at least 4.4 (or even
> beforehand).

Exactly, one can verify that by looking at guessed branch probabilities:

$ cat /tmp/tc.c
int
main(int argc)
{
   if (argc == 123)
     __builtin_abort ();
   else
     return 0;
}

$ gcc /tmp/tc.c -fdump-tree-optimized=/dev/stdout -O2
...
   if (argc_1(D) == 123)
     goto <bb 3>; [0.00%]
   else
     goto <bb 4>; [100.00%]

One can see more details in -fdump-tree-profile_estimate-details dump file.

Martin
  

On Tue, Jul 28, 2020 at 10:19 PM Aditya K via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> > This is not needed for GCC to do the correct thing as __assert_fail is
> > marked as noreturn and GCC heuristics for branch prediction already
> > predicts it correctly and has done since at least 4.4 (or even
> > beforehand).
> > Can you explain more on which compiler this helps with?
>
> I saw this pattern in 'MacOSX.sdk/usr/include/assert.h', so added here. I don't have any test case to show a different layout with this patch.

That suggests that some versions of clang might not have the "noreturn
paths are cold" heuristic.  Can you test whether anything changes with
your patch on Linux with clang?  Try several different versions if
possible.

I want to be clear that we're _not_ rejecting your patch out of hand.
It's just that we don't want to add annotations that don't make any
difference for code generation.  If there is a compiler that's
commonly used with glibc, that either doesn't have the "noreturn paths
are cold" heuristic or doesn't understand that __assert_fail etc never
return, then we would take changes to assert.h to improve code
generation with that compiler.

zw
  

On Wed, 29 Jul 2020, Zack Weinberg wrote:

> I want to be clear that we're _not_ rejecting your patch out of hand.
> It's just that we don't want to add annotations that don't make any
> difference for code generation.  If there is a compiler that's
> commonly used with glibc, that either doesn't have the "noreturn paths
> are cold" heuristic or doesn't understand that __assert_fail etc never
> return, then we would take changes to assert.h to improve code
> generation with that compiler.

Note that in any case where annotations turn out to be useful, we should 
use __glibc_likely / __glibc_unlikely (for any true / false conditional) 
rather than using __builtin_expect expect directly.
  

So I tried with fairly latest clang (trunk June) and removing (noreturn) from assert_fail does change the basic block layout at -O2/-O3. So it seems this change may not be needed.

I tried compiling 'https://github.com/llvm/llvm-project/blob/master/clang/test/Analysis/null-deref-ps.c' which is self-contained with a definition of the assert macro.

A reduced test example is here:
https://godbolt.org/z/WeTcMj


-Aditya


From: Joseph Myers <joseph@codesourcery.com>
Sent: Wednesday, July 29, 2020 10:00 AM
To: Zack Weinberg <zackw@panix.com>
Cc: Aditya K <hiraditya@msn.com>; libc-alpha@sourceware.org <libc-alpha@sourceware.org>; Sebastian Pop <sebpop@gmail.com>
Subject: Re: Add builtin likely to assertions 
 
On Wed, 29 Jul 2020, Zack Weinberg wrote:

> I want to be clear that we're _not_ rejecting your patch out of hand.
> It's just that we don't want to add annotations that don't make any
> difference for code generation.  If there is a compiler that's
> commonly used with glibc, that either doesn't have the "noreturn paths
> are cold" heuristic or doesn't understand that __assert_fail etc never
> return, then we would take changes to assert.h to improve code
> generation with that compiler.

Note that in any case where annotations turn out to be useful, we should 
use __glibc_likely / __glibc_unlikely (for any true / false conditional) 
rather than using __builtin_expect expect directly.