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

[Bruno Ricci via Phabricator via cfe-commits]

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