riccibruno added a subscriber: NoQ.
riccibruno added a comment.
It seems that the tests are not present in this diff ? Also, again, could you
please:
1. Use `clang-format`, and
2. Make sure that the comments are full sentences with appropriate punctuation,
and
3. Follow the style guide regarding for the names of variables and functions.
================
Comment at: clang/include/clang/AST/Stmt.h:63
+ NotTaken = 2
+};
+
----------------
Can you make this a scoped enumeration to avoid injecting these names
everywhere ? (+ add a comment describing what it is used for)
================
Comment at: clang/include/clang/Sema/Scope.h:168
+ /// BranchAttr - This is the Likelihood attribute associated with this
Branch or a nullptr.
+ LikelihoodAttr *BranchAttr;
+
----------------
Perhaps `BranchAttr` -> `BranchLikelihoodAttr` ?
================
Comment at: clang/lib/AST/Stmt.cpp:832
IfStmtBits.HasInit = HasInit;
+ IfStmtBits.Hint = NoHint;
}
----------------
A small remark: there is no need to initialize it here since this will be done
during deserialization. Not initializing it here has the advantage that
forgetting the initialization during deserialization will (hopefully) cause an
msan failure. The above bits are special since they are needed to correctly
access the trailing objects.
================
Comment at: clang/lib/Analysis/CFG.cpp:2208
+}
+
CFGBlock *CFGBuilder::VisitStmt(Stmt *S, AddStmtChoice asc) {
----------------
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 ?)
================
Comment at: clang/lib/CodeGen/CGStmt.cpp:705
+}
+
void CodeGenFunction::EmitWhileStmt(const WhileStmt &S,
----------------
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`.
================
Comment at: clang/lib/Parse/ParseStmt.cpp:1262
+ LikelihoodAttr *ThenAttr = getCurScope()->getBranchAttr();
+ getCurScope()->setBranchAttr(nullptr);
----------------
Perhaps `ThenAttr` -> `ThenLikelihoodAttr` ?
================
Comment at: clang/lib/Parse/ParseStmt.cpp:1304
}
+ LikelihoodAttr *ElseAttr = getCurScope()->getBranchAttr();
----------------
Perhaps `ElseAttr` -> `ElseLikelihoodAttr` ?
================
Comment at: clang/lib/Sema/SemaStmt.cpp:524
-StmtResult
-Sema::ActOnIfStmt(SourceLocation IfLoc, bool IsConstexpr, Stmt *InitStmt,
- ConditionResult Cond,
- Stmt *thenStmt, SourceLocation ElseLoc,
- Stmt *elseStmt) {
+BranchHint Sema::HandleIfStmtHint(Stmt *ThenStmt, Stmt *elseStmt,
+ LikelihoodAttr *ThenAttr, LikelihoodAttr
*ElseAttr) {
----------------
I would appreciate some documentation above `HandleIfStmtHint`.
================
Comment at: clang/lib/Sema/SemaStmt.cpp:529
+ if (ThenAttr) {
+ if (ElseAttr && ThenAttr->getSpelling()[0] == ElseAttr->getSpelling()[0]) {
+ Diag(ElseAttr->getLocation(), diag::warn_conflicting_likelihood_attrs)
----------------
Do you have to use `getSpelling()` here ? Why not use `isLikely()` and
`isUnlikely()` as below ?
================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:63
+ if (isa<CaseStmt>(St) || isa<DefaultStmt>(St)) {
+ auto *FnScope = S.getCurFunction();
+ if (FnScope->SwitchStack.empty()) {
----------------
Type not obvious -> no auto please.
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
