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

[Gauthier via Phabricator via cfe-commits]

Tyker marked 2 inline comments as done.
Tyker added inline comments.


================
Comment at: clang/lib/Analysis/CFG.cpp:2208
+}
+
 CFGBlock *CFGBuilder::VisitStmt(Stmt *S, AddStmtChoice asc) {
----------------
riccibruno wrote:
> I don't understand why this is needed. Can you explain it ? Also I think that someone familiar with this code should comment on this (maybe @NoQ ?)
the detail of why are complicated and i don't have them all in head but without this edit in cases like 

```
switch (...) {
[[likely]] case 1:
        ...
        [[fallthrough]];
default:
        ...
}
```
the fallthrough attribute emitted a diagnostic because is wasn't handling attributed case statement. the edit i performed is probably not the optimal way to solve the issue as it only solves the issue for likelihood attribute. but i don't know any other attribute that can be applied on a case statement but if they were others they would probably have the same issue. but the code is quite hard to follow and i didn't wanted to break anything. so this is what i came up with.
i am going to look into it to find a better solution.


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:705
+}
+
 void CodeGenFunction::EmitWhileStmt(const WhileStmt &S,
----------------
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.


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

https://reviews.llvm.org/D59467