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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
