[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:705
+}
+
 void CodeGenFunction::EmitWhileStmt(const WhileStmt &S,
----------------
Tyker wrote:
> riccibruno wrote:
> > I believe that the lowering is incorrect. I applied your patch and here ({F8571803}) is the IR that clang generates (obtained with `-O1 -S -emit-llvm -Xclang -disable-llvm-passes -g0`) for this code:
> > 
> > ```
> > bool f(bool i);
> > bool g(bool i);
> > 
> > bool h1(bool i) {
> >   if (i) [[likely]]
> >     return f(i);
> >   return g(i);
> > }
> > 
> > bool h2(bool i) {
> >   if (__builtin_expect(i, true))
> >     return f(i);
> >   return g(i);
> > }
> > ```
> > 
> > In particular for the branch in `h1` we have:
> > ```
> >   %tobool = trunc i8 %0 to i1
> >   %expval = call i1 @llvm.expect.i1(i1 %tobool, i1 true)
> >   br i1 %tobool, label %if.then, label %if.end
> > ```
> > Note that `%expval` is not used. Compare this to the branch in `h2`:
> > ```
> >   %tobool = trunc i8 %0 to i1
> >   %conv = zext i1 %tobool to i64
> >   %expval = call i64 @llvm.expect.i64(i64 %conv, i64 1)
> >   %tobool1 = icmp ne i64 %expval, 0
> >   br i1 %tobool1, label %if.then, label %if.end
> > ```
> > where the extra conversions are because of the signature of `__builtin_expect`.
> from reading the documentation it seemed to me that both were equivalent. but after further checking there aren't.
Looking at how the intrinsic is lowered in the `LowerExpectIntrinsic` pass (in `handleBrSelExpect`), it seems that for a branch or select the following patterns are supported:

```
  // Handle non-optimized IR code like:
  //   %expval = call i64 @llvm.expect.i64(i64 %conv1, i64 1)
  //   %tobool = icmp ne i64 %expval, 0
  //   br i1 %tobool, label %if.then, label %if.end
  //
  // Or the following simpler case:
  //   %expval = call i1 @llvm.expect.i1(i1 %cmp, i1 1)
  //   br i1 %expval, label %if.then, label %if.end
```




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59467/new/

https://reviews.llvm.org/D59467