[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a
Tyker updated this revision to Diff 192811.
Tyker retitled this revision from "[clang] Adding the Likely Attribute from
C++2a to AST" to "[clang] Adding the Likelihood Attribute from C++2a".
Tyker added a comment.
@aaron.ballman fixed based on feedback.
added semantic support for switch statment.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59467/new/
https://reviews.llvm.org/D59467
Files:
clang/include/clang/AST/Stmt.h
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/AttrDocs.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Scope.h
clang/include/clang/Sema/Sema.h
clang/lib/AST/Stmt.cpp
clang/lib/AST/TextNodeDumper.cpp
clang/lib/Analysis/CFG.cpp
clang/lib/CodeGen/CGStmt.cpp
clang/lib/CodeGen/CodeGenFunction.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/Parse/ParseStmt.cpp
clang/lib/Sema/Scope.cpp
clang/lib/Sema/SemaStmt.cpp
clang/lib/Sema/SemaStmtAttr.cpp
clang/lib/Serialization/ASTReaderStmt.cpp
clang/lib/Serialization/ASTWriterStmt.cpp
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -139,6 +139,7 @@
Record.push_back(HasElse);
Record.push_back(HasVar);
Record.push_back(HasInit);
+ Record.push_back(S->getBranchHint());
Record.AddStmt(S->getCond());
Record.AddStmt(S->getThen());
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -222,6 +222,7 @@
bool HasElse = Record.readInt();
bool HasVar = Record.readInt();
bool HasInit = Record.readInt();
+ S->setBranchHint(static_cast(Record.readInt()));
S->setCond(Record.readSubExpr());
S->setThen(Record.readSubStmt());
Index: clang/lib/Sema/SemaStmtAttr.cpp
===
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -51,6 +51,55 @@
return ::new (S.Context) auto(Attr);
}
+static Attr *handleLikelihoodAttr(Sema &S, Stmt *St, const ParsedAttr &A,
+ SourceRange Range) {
+ LikelihoodAttr *Attr = ::new (S.Context) LikelihoodAttr(
+ A.getRange(), S.Context, A.getAttributeSpellingListIndex());
+
+ if (!S.getLangOpts().CPlusPlus2a && A.isCXX11Attribute())
+S.Diag(A.getLoc(), diag::ext_cxx2a_attr) << A.getName();
+
+ if (isa(St) || isa(St)) {
+auto *FnScope = S.getCurFunction();
+if (FnScope->SwitchStack.empty()) {
+ S.Diag(A.getLoc(), diag::warn_likelihood_on_case_outside_switch) << Attr->getSpelling()
+<< (isa(St) ? "case" : "default");
+}
+return Attr;
+ }
+
+ Scope *CurScope = S.getCurScope();
+ if (CurScope) {
+Scope *ControlScope = CurScope->getParent();
+if (!ControlScope ||
+ !(ControlScope->getFlags() & Scope::ControlScope ||
+ ControlScope->getFlags() & Scope::BreakScope) ||
+ ControlScope->getFlags() & Scope::SEHExceptScope ||
+ ControlScope->getFlags() & Scope::SEHTryScope ||
+ ControlScope->getFlags() & Scope::TryScope ||
+ ControlScope->getFlags() & Scope::FnTryCatchScope) {
+ S.Diag(A.getLoc(), diag::warn_no_likelihood_attr_associated_branch)
+ << A.getName();
+ return Attr;
+}
+
+if (ControlScope->getBranchAttr()) {
+ if (ControlScope->getBranchAttr()->getSpelling()[0] ==
+ Attr->getSpelling()[0])
+S.Diag(Attr->getLocation(), diag::err_repeat_attribute)
+<< Attr->getSpelling();
+ else
+S.Diag(Attr->getLocation(), diag::err_attributes_are_not_compatible)
+<< Attr->getSpelling()
+<< ControlScope->getBranchAttr()->getSpelling();
+ S.Diag(ControlScope->getBranchAttr()->getLocation(),
+ diag::note_previous_attribute);
+} else
+ ControlScope->setBranchAttr(Attr);
+ }
+ return Attr;
+}
+
static Attr *handleSuppressAttr(Sema &S, Stmt *St, const ParsedAttr &A,
SourceRange Range) {
if (A.getNumArgs() < 1) {
@@ -336,6 +385,8 @@
return nullptr;
case ParsedAttr::AT_FallThrough:
return handleFallThroughAttr(S, St, A, Range);
+ case ParsedAttr::AT_Likelihood:
+return handleLikelihoodAttr(S, St, A, Range);
case ParsedAttr::AT_LoopHint:
return handleLoopHintAttr(S, St, A, Range);
case ParsedAttr::AT_OpenCLUnrollHint:
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -521,11 +521,27 @@
};
}
-StmtResult
-Sema::ActOnIfStmt(SourceLocation IfLoc, bool IsConstexpr, Stmt *InitStmt,
- ConditionResult Cond,
- Stmt *thenStmt, SourceLocation ElseLoc,
-
[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a
Tyker updated this revision to Diff 192979.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59467/new/
https://reviews.llvm.org/D59467
Files:
clang/include/clang/AST/Stmt.h
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/AttrDocs.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Scope.h
clang/include/clang/Sema/Sema.h
clang/lib/AST/Stmt.cpp
clang/lib/AST/TextNodeDumper.cpp
clang/lib/Analysis/CFG.cpp
clang/lib/CodeGen/CGStmt.cpp
clang/lib/CodeGen/CodeGenFunction.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/Parse/ParseStmt.cpp
clang/lib/Sema/Scope.cpp
clang/lib/Sema/SemaStmt.cpp
clang/lib/Sema/SemaStmtAttr.cpp
clang/lib/Serialization/ASTReaderStmt.cpp
clang/lib/Serialization/ASTWriterStmt.cpp
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -140,6 +140,7 @@
Record.push_back(HasElse);
Record.push_back(HasVar);
Record.push_back(HasInit);
+ Record.push_back(S->getBranchHint());
Record.AddStmt(S->getCond());
Record.AddStmt(S->getThen());
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -223,6 +223,7 @@
bool HasElse = Record.readInt();
bool HasVar = Record.readInt();
bool HasInit = Record.readInt();
+ S->setBranchHint(static_cast(Record.readInt()));
S->setCond(Record.readSubExpr());
S->setThen(Record.readSubStmt());
Index: clang/lib/Sema/SemaStmtAttr.cpp
===
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -51,6 +51,55 @@
return ::new (S.Context) auto(Attr);
}
+static Attr *handleLikelihoodAttr(Sema &S, Stmt *St, const ParsedAttr &A,
+ SourceRange Range) {
+ LikelihoodAttr *Attr = ::new (S.Context) LikelihoodAttr(
+ A.getRange(), S.Context, A.getAttributeSpellingListIndex());
+
+ if (!S.getLangOpts().CPlusPlus2a && A.isCXX11Attribute())
+S.Diag(A.getLoc(), diag::ext_cxx2a_attr) << A.getName();
+
+ if (isa(St) || isa(St)) {
+auto *FnScope = S.getCurFunction();
+if (FnScope->SwitchStack.empty()) {
+ S.Diag(A.getLoc(), diag::warn_likelihood_on_case_outside_switch) << Attr->getSpelling()
+<< (isa(St) ? "case" : "default");
+}
+return Attr;
+ }
+
+ Scope *CurScope = S.getCurScope();
+ if (CurScope) {
+Scope *ControlScope = CurScope->getParent();
+if (!ControlScope ||
+ !(ControlScope->getFlags() & Scope::ControlScope ||
+ ControlScope->getFlags() & Scope::BreakScope) ||
+ ControlScope->getFlags() & Scope::SEHExceptScope ||
+ ControlScope->getFlags() & Scope::SEHTryScope ||
+ ControlScope->getFlags() & Scope::TryScope ||
+ ControlScope->getFlags() & Scope::FnTryCatchScope) {
+ S.Diag(A.getLoc(), diag::warn_no_likelihood_attr_associated_branch)
+ << A.getName();
+ return Attr;
+}
+
+if (ControlScope->getBranchAttr()) {
+ if (ControlScope->getBranchAttr()->getSpelling()[0] ==
+ Attr->getSpelling()[0])
+S.Diag(Attr->getLocation(), diag::err_repeat_attribute)
+<< Attr->getSpelling();
+ else
+S.Diag(Attr->getLocation(), diag::err_attributes_are_not_compatible)
+<< Attr->getSpelling()
+<< ControlScope->getBranchAttr()->getSpelling();
+ S.Diag(ControlScope->getBranchAttr()->getLocation(),
+ diag::note_previous_attribute);
+} else
+ ControlScope->setBranchAttr(Attr);
+ }
+ return Attr;
+}
+
static Attr *handleSuppressAttr(Sema &S, Stmt *St, const ParsedAttr &A,
SourceRange Range) {
if (A.getNumArgs() < 1) {
@@ -336,6 +385,8 @@
return nullptr;
case ParsedAttr::AT_FallThrough:
return handleFallThroughAttr(S, St, A, Range);
+ case ParsedAttr::AT_Likelihood:
+return handleLikelihoodAttr(S, St, A, Range);
case ParsedAttr::AT_LoopHint:
return handleLoopHintAttr(S, St, A, Range);
case ParsedAttr::AT_OpenCLUnrollHint:
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -521,11 +521,27 @@
};
}
-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) {
+ BranchHint Hint = NoHint;
+ // diagnose conflicting attribute and
[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a
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(St) || isa(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
[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a
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
[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a
Tyker updated this revision to Diff 193015.
Tyker added a comment.
Herald added a reviewer: martong.
Herald added a reviewer: shafik.
@riccibruno i fixed based on feedback everything except the CFG edit as i still
need to analyse the situation.
added AST and CodeGen for For, While, Do and CXXFor.
added tests for Semantic, AST, PCH
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59467/new/
https://reviews.llvm.org/D59467
Files:
clang/include/clang/AST/Stmt.h
clang/include/clang/AST/StmtCXX.h
clang/include/clang/AST/TextNodeDumper.h
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/AttrDocs.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Scope.h
clang/include/clang/Sema/Sema.h
clang/lib/AST/ASTImporter.cpp
clang/lib/AST/Stmt.cpp
clang/lib/AST/StmtCXX.cpp
clang/lib/AST/TextNodeDumper.cpp
clang/lib/Analysis/CFG.cpp
clang/lib/CodeGen/CGStmt.cpp
clang/lib/CodeGen/CodeGenFunction.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/Parse/ParseStmt.cpp
clang/lib/Sema/Scope.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaStmt.cpp
clang/lib/Sema/SemaStmtAttr.cpp
clang/lib/Sema/TreeTransform.h
clang/lib/Serialization/ASTReaderStmt.cpp
clang/lib/Serialization/ASTWriterStmt.cpp
clang/test/CodeGenCXX/cxx2a-likelihood-attr.cpp
clang/test/PCH/cxx2a-likelihood-attr.cpp
clang/test/SemaCXX/cxx17-compat.cpp
clang/test/SemaCXX/cxx2a-likelihood-attr.cpp
Index: clang/test/SemaCXX/cxx2a-likelihood-attr.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/cxx2a-likelihood-attr.cpp
@@ -0,0 +1,70 @@
+// RUN: %clang_cc1 -fsyntax-only %s -verify -std=c++2a
+
+// formating this file will break the test
+// clang-format off
+
+int f(int);
+
+int f(int i) {
+ if (i == 1)
+[[unlikely]] { f(i); }
+ else if (i == 2)
+[[likely]] return f(i);
+ else
+[[unlikely]] { return f(i + 1); }
+ return i;
+}
+
+[[likely]] typedef int n1;
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+typedef int [[likely]] n2;
+// expected-error@-1 {{'likely' attribute cannot be applied to types}}
+typedef int n3 [[likely]];
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+
+enum [[likely]] E{One};
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+
+[[likely]]
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+
+void test(int i) {
+ [[likely]]
+ // expected-warning@-1 {{attribute 'likely' is not associated with a branch and is ignored}}
+ if (f(i)) [[likely, likely]] {
+// expected-error@-1 {{likely attribute cannot be repeated}}
+// expected-note@-2 {{previous attribute is here}}
+[[unlikely]] return;
+// expected-warning@-1 {{attribute 'unlikely' is not associated with a branch and is ignored}}
+ }
+ else [[unlikely]] if (f(i)) {
+while (f(i))
+ [[likely]] {
+switch (i) {
+ [[likely]] case 1 : default : [[likely]];
+ // expected-warning@-1 {{attribute 'likely' is not associated with a branch and is ignored}}
+ [[unlikely]] case 3 : f(i);
+ [[fallthrough]];
+case 4:
+ return;
+}
+ }
+for (;;)
+ [[likely, unlikely]]
+ // expected-error@-1 {{unlikely and likely attributes are not compatible}}
+ // expected-note@-2 {{previous attribute is here}}
+ [[likely]] return;
+// expected-error@-1 {{likely attribute cannot be repeated}}
+// expected-note@-5 {{previous attribute is here}}
+ }
+ try { // expected-error {{cannot use 'try' with exceptions disabled}}
+[[likely]];
+// expected-warning@-1 {{attribute 'likely' is not associated with a branch and is ignored}}
+ } catch (int) {
+[[likely]] test :
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+[[unlikely]] return;
+ }
+}
+
+// clang-format off
Index: clang/test/SemaCXX/cxx17-compat.cpp
===
--- clang/test/SemaCXX/cxx17-compat.cpp
+++ clang/test/SemaCXX/cxx17-compat.cpp
@@ -56,10 +56,33 @@
};
void ForRangeInit() {
- for (int arr[3] = {1, 2, 3}; int n : arr) {}
+ for (int arr[3] = {1, 2, 3}; int n : arr) {
+ }
#if __cplusplus <= 201703L
-// expected-warning@-2 {{range-based for loop initialization statements are a C++2a extension}}
+ // expected-warning@-2 {{range-based for loop initialization statements are a
+ // C++2a extension}}
#else
-// expected-warning@-4 {{range-based for loop initialization statements are incompatible with C++ standards before C++2a}}
+ // expected-warning@-4 {{range-based for loop initialization statements are
+ // incompatible with C++ standards before C++2a}}
+#endif
+}
+
+int f(int i) {
+ if (i == 1)
+[[unlikely]]
+#if __cplusplus <= 201703L
+// expected-warning@-2 {{use of the 'unlikely' attribute is a
[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a
Tyker marked an inline comment as done. Tyker added inline comments. Herald added a subscriber: rnkovacs. Comment at: clang/test/SemaCXX/cxx2a-likelihood-attr.cpp:70 + +// clang-format off just saw the //clang format off at the bottom ill remove it 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
[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a
Tyker marked an inline comment as done.
Tyker added inline comments.
Comment at: clang/lib/Sema/SemaStmt.cpp:528
+if (ElseLikelihoodAttr && ThenLikelihoodAttr->isEqual(ElseLikelihoodAttr))
{
+ Diag(ElseLikelihoodAttr->getLocation(),
+ diag::warn_conflicting_likelihood_attrs)
clang-format passed on the whole file i am fixing it
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
[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a
Tyker updated this revision to Diff 193016.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59467/new/
https://reviews.llvm.org/D59467
Files:
clang/include/clang/AST/Stmt.h
clang/include/clang/AST/StmtCXX.h
clang/include/clang/AST/TextNodeDumper.h
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/AttrDocs.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Scope.h
clang/include/clang/Sema/Sema.h
clang/lib/AST/ASTImporter.cpp
clang/lib/AST/Stmt.cpp
clang/lib/AST/StmtCXX.cpp
clang/lib/AST/TextNodeDumper.cpp
clang/lib/Analysis/CFG.cpp
clang/lib/CodeGen/CGStmt.cpp
clang/lib/CodeGen/CodeGenFunction.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/Parse/ParseStmt.cpp
clang/lib/Sema/Scope.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaStmt.cpp
clang/lib/Sema/SemaStmtAttr.cpp
clang/lib/Sema/TreeTransform.h
clang/lib/Serialization/ASTReaderStmt.cpp
clang/lib/Serialization/ASTWriterStmt.cpp
clang/test/CodeGenCXX/cxx2a-likelihood-attr.cpp
clang/test/PCH/cxx2a-likelihood-attr.cpp
clang/test/SemaCXX/cxx17-compat.cpp
clang/test/SemaCXX/cxx2a-likelihood-attr.cpp
Index: clang/test/SemaCXX/cxx2a-likelihood-attr.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/cxx2a-likelihood-attr.cpp
@@ -0,0 +1,68 @@
+// RUN: %clang_cc1 -fsyntax-only %s -verify -std=c++2a
+
+// formating this file will break the test
+// clang-format off
+
+int f(int);
+
+int f(int i) {
+ if (i == 1)
+[[unlikely]] { f(i); }
+ else if (i == 2)
+[[likely]] return f(i);
+ else
+[[unlikely]] { return f(i + 1); }
+ return i;
+}
+
+[[likely]] typedef int n1;
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+typedef int [[likely]] n2;
+// expected-error@-1 {{'likely' attribute cannot be applied to types}}
+typedef int n3 [[likely]];
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+
+enum [[likely]] E{One};
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+
+[[likely]]
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+
+void test(int i) {
+ [[likely]]
+ // expected-warning@-1 {{attribute 'likely' is not associated with a branch and is ignored}}
+ if (f(i)) [[likely, likely]] {
+// expected-error@-1 {{likely attribute cannot be repeated}}
+// expected-note@-2 {{previous attribute is here}}
+[[unlikely]] return;
+// expected-warning@-1 {{attribute 'unlikely' is not associated with a branch and is ignored}}
+ }
+ else [[unlikely]] if (f(i)) {
+while (f(i))
+ [[likely]] {
+switch (i) {
+ [[likely]] case 1 : default : [[likely]];
+ // expected-warning@-1 {{attribute 'likely' is not associated with a branch and is ignored}}
+ [[unlikely]] case 3 : f(i);
+ [[fallthrough]];
+case 4:
+ return;
+}
+ }
+for (;;)
+ [[likely, unlikely]]
+ // expected-error@-1 {{unlikely and likely attributes are not compatible}}
+ // expected-note@-2 {{previous attribute is here}}
+ [[likely]] return;
+// expected-error@-1 {{likely attribute cannot be repeated}}
+// expected-note@-5 {{previous attribute is here}}
+ }
+ try { // expected-error {{cannot use 'try' with exceptions disabled}}
+[[likely]];
+// expected-warning@-1 {{attribute 'likely' is not associated with a branch and is ignored}}
+ } catch (int) {
+[[likely]] test :
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+[[unlikely]] return;
+ }
+}
\ No newline at end of file
Index: clang/test/SemaCXX/cxx17-compat.cpp
===
--- clang/test/SemaCXX/cxx17-compat.cpp
+++ clang/test/SemaCXX/cxx17-compat.cpp
@@ -56,10 +56,33 @@
};
void ForRangeInit() {
- for (int arr[3] = {1, 2, 3}; int n : arr) {}
+ for (int arr[3] = {1, 2, 3}; int n : arr) {
+ }
#if __cplusplus <= 201703L
-// expected-warning@-2 {{range-based for loop initialization statements are a C++2a extension}}
+ // expected-warning@-2 {{range-based for loop initialization statements are a
+ // C++2a extension}}
#else
-// expected-warning@-4 {{range-based for loop initialization statements are incompatible with C++ standards before C++2a}}
+ // expected-warning@-4 {{range-based for loop initialization statements are
+ // incompatible with C++ standards before C++2a}}
+#endif
+}
+
+int f(int i) {
+ if (i == 1)
+[[unlikely]]
+#if __cplusplus <= 201703L
+// expected-warning@-2 {{use of the 'unlikely' attribute is a C++2a
+// extension}}
+#endif
+{
+ f(i);
+}
+ else if (i == 2)
+[[likely]]
+#if __cplusplus <= 201703L
+// expected-warning@-2 {{use of the 'likely' attribute is a C++2a
+// extension}}
#endif
+return f(i);
+ return i;
}
Index: clang/test/PCH/cxx2a-l
[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a
NoQ added inline comments.
Comment at: clang/lib/Analysis/CFG.cpp:2208
+}
+
CFGBlock *CFGBuilder::VisitStmt(Stmt *S, AddStmtChoice asc) {
Tyker wrote:
> 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.
The [[likely]] attribute should not affect the overall topology of the CFG. It
might be a nice piece of metadata to add to a CFG edge or to a CFG terminator,
but for most consumers of the CFG (various static analyses such as
analysis-based warnings or the Static Analyzer) the attribute should have
little to no effect - the tool would still need to explore both branches. I
don't know how exactly the fallthrough warning operates, but i find it likely
(no pun intended) that the fallthrough warning itself should be updated, not
the CFG.
It is probable that for compiler warnings it'll only cause false negatives,
which is not as bad as false positives, but i wouldn't rely on that.
Additionally, false negatives in such rare scenarios will be very hard to
notice later. So i'm highly in favor of aiming for the correct solution in this
patch.
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
[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a
Tyker marked an inline comment as done.
Tyker added inline comments.
Comment at: clang/lib/Analysis/CFG.cpp:2208
+}
+
CFGBlock *CFGBuilder::VisitStmt(Stmt *S, AddStmtChoice asc) {
NoQ wrote:
> Tyker wrote:
> > 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.
> The [[likely]] attribute should not affect the overall topology of the CFG.
> It might be a nice piece of metadata to add to a CFG edge or to a CFG
> terminator, but for most consumers of the CFG (various static analyses such
> as analysis-based warnings or the Static Analyzer) the attribute should have
> little to no effect - the tool would still need to explore both branches. I
> don't know how exactly the fallthrough warning operates, but i find it likely
> (no pun intended) that the fallthrough warning itself should be updated, not
> the CFG.
>
> It is probable that for compiler warnings it'll only cause false negatives,
> which is not as bad as false positives, but i wouldn't rely on that.
> Additionally, false negatives in such rare scenarios will be very hard to
> notice later. So i'm highly in favor of aiming for the correct solution in
> this patch.
>
>
i think we all agree that the CFG structure shouldn't be affected by the
presence or absence of the likely attribute. but in the current state(no
changes to the CFG) it does for example.
the CFG were obtained without the edit in CFG.cpp but with the added likely
attribute
using: clang -cc1 -analyze test.cpp -analyzer-checker=debug.DumpCFG
input:
```
int f(int i) {
switch (i) {
[[likely]] case 1:
return 1;
}
return i;
}
```
outputs:
```
[B5 (ENTRY)]
Succs (1): B2
[B1]
1: i
2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
3: return [B1.2];
Preds (2): B3 B2
Succs (1): B0
[B2]
1: i
2: [B2.1] (ImplicitCastExpr, LValueToRValue, int)
T: switch [B2.2]
Preds (1): B5
Succs (2): B4 B1
[B3]
1: [[likely]]case 1:
[B4.2] Succs (1): B1
[B4]
case 1:
1: 1
2: return [B4.1];
Preds (1): B2
Succs (1): B0
[B0 (EXIT)]
Preds (2): B1 B4
```
and
input:
```
int f(int i) {
switch (i) {
case 1:
return 1;
}
return i;
}
```
outputs:
```
[B4 (ENTRY)]
Succs (1): B2
[B1]
1: i
2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
3: return [B1.2];
Preds (1): B2
Succs (1): B0
[B2]
1: i
2: [B2.1] (ImplicitCastExpr, LValueToRValue, int)
T: switch [B2.2]
Preds (1): B4
Succs (2): B3 B1
[B3]
case 1:
1: 1
2: return [B3.1];
Preds (1): B2
Succs (1): B0
[B0 (EXIT)]
Preds (2): B1 B3
```
i think think this is the underlying issue. the false diagnostic from
fallthrough previously mentioned is a consequence of this
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
[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a
NoQ added inline comments.
Comment at: clang/lib/Analysis/CFG.cpp:2208
+}
+
CFGBlock *CFGBuilder::VisitStmt(Stmt *S, AddStmtChoice asc) {
Tyker wrote:
> NoQ wrote:
> > Tyker wrote:
> > > 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.
> > The [[likely]] attribute should not affect the overall topology of the CFG.
> > It might be a nice piece of metadata to add to a CFG edge or to a CFG
> > terminator, but for most consumers of the CFG (various static analyses such
> > as analysis-based warnings or the Static Analyzer) the attribute should
> > have little to no effect - the tool would still need to explore both
> > branches. I don't know how exactly the fallthrough warning operates, but i
> > find it likely (no pun intended) that the fallthrough warning itself should
> > be updated, not the CFG.
> >
> > It is probable that for compiler warnings it'll only cause false negatives,
> > which is not as bad as false positives, but i wouldn't rely on that.
> > Additionally, false negatives in such rare scenarios will be very hard to
> > notice later. So i'm highly in favor of aiming for the correct solution in
> > this patch.
> >
> >
> i think we all agree that the CFG structure shouldn't be affected by the
> presence or absence of the likely attribute. but in the current state(no
> changes to the CFG) it does for example.
>
> the CFG were obtained without the edit in CFG.cpp but with the added likely
> attribute
> using: clang -cc1 -analyze test.cpp -analyzer-checker=debug.DumpCFG
>
> input:
>
> ```
> int f(int i) {
> switch (i) {
> [[likely]] case 1:
> return 1;
> }
> return i;
> }
>
> ```
> outputs:
>
> ```
> [B5 (ENTRY)]
>Succs (1): B2
>
> [B1]
>1: i
>2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
>3: return [B1.2];
>Preds (2): B3 B2
>Succs (1): B0
>
> [B2]
>1: i
>2: [B2.1] (ImplicitCastExpr, LValueToRValue, int)
>T: switch [B2.2]
>Preds (1): B5
>Succs (2): B4 B1
>
> [B3]
>1: [[likely]]case 1:
> [B4.2] Succs (1): B1
>
> [B4]
> case 1:
>1: 1
>2: return [B4.1];
>Preds (1): B2
>Succs (1): B0
>
> [B0 (EXIT)]
>Preds (2): B1 B4
>
> ```
> and
> input:
>
> ```
> int f(int i) {
> switch (i) {
> case 1:
> return 1;
> }
> return i;
> }
>
> ```
> outputs:
>
> ```
> [B4 (ENTRY)]
>Succs (1): B2
>
> [B1]
>1: i
>2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
>3: return [B1.2];
>Preds (1): B2
>Succs (1): B0
>
> [B2]
>1: i
>2: [B2.1] (ImplicitCastExpr, LValueToRValue, int)
>T: switch [B2.2]
>Preds (1): B4
>Succs (2): B3 B1
>
> [B3]
> case 1:
>1: 1
>2: return [B3.1];
>Preds (1): B2
>Succs (1): B0
>
> [B0 (EXIT)]
>Preds (2): B1 B3
> ```
> i think think this is the underlying issue. the false diagnostic from
> fallthrough previously mentioned is a consequence of this
Hmm, i see. I got it all wrong. Please forgive me!
You're just trying to make `CFGBuilder` support `AttributedStmt` correctly in
general. And the logic you're writing says "support it as any other generic
Stmt that doesn't have any control flow in it, unless it's a
`[[likely]]`-attributed `AttributedStmt`, in which case jump straight to
children".
Could you instead do this by implementing `CFGBuilder::VisitAttributedStmt`
with that logic?
I'm also not sure this logic is actually specific to `[[likely]]`. Maybe we
should unwrap all `AttributedStmt`s similarly?
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
[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a
Tyker marked an inline comment as done.
Tyker added inline comments.
Comment at: clang/lib/Analysis/CFG.cpp:2208
+}
+
CFGBlock *CFGBuilder::VisitStmt(Stmt *S, AddStmtChoice asc) {
NoQ wrote:
> Tyker wrote:
> > NoQ wrote:
> > > Tyker wrote:
> > > > 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.
> > > The [[likely]] attribute should not affect the overall topology of the
> > > CFG. It might be a nice piece of metadata to add to a CFG edge or to a
> > > CFG terminator, but for most consumers of the CFG (various static
> > > analyses such as analysis-based warnings or the Static Analyzer) the
> > > attribute should have little to no effect - the tool would still need to
> > > explore both branches. I don't know how exactly the fallthrough warning
> > > operates, but i find it likely (no pun intended) that the fallthrough
> > > warning itself should be updated, not the CFG.
> > >
> > > It is probable that for compiler warnings it'll only cause false
> > > negatives, which is not as bad as false positives, but i wouldn't rely on
> > > that. Additionally, false negatives in such rare scenarios will be very
> > > hard to notice later. So i'm highly in favor of aiming for the correct
> > > solution in this patch.
> > >
> > >
> > i think we all agree that the CFG structure shouldn't be affected by the
> > presence or absence of the likely attribute. but in the current state(no
> > changes to the CFG) it does for example.
> >
> > the CFG were obtained without the edit in CFG.cpp but with the added likely
> > attribute
> > using: clang -cc1 -analyze test.cpp -analyzer-checker=debug.DumpCFG
> >
> > input:
> >
> > ```
> > int f(int i) {
> > switch (i) {
> > [[likely]] case 1:
> > return 1;
> > }
> > return i;
> > }
> >
> > ```
> > outputs:
> >
> > ```
> > [B5 (ENTRY)]
> >Succs (1): B2
> >
> > [B1]
> >1: i
> >2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
> >3: return [B1.2];
> >Preds (2): B3 B2
> >Succs (1): B0
> >
> > [B2]
> >1: i
> >2: [B2.1] (ImplicitCastExpr, LValueToRValue, int)
> >T: switch [B2.2]
> >Preds (1): B5
> >Succs (2): B4 B1
> >
> > [B3]
> >1: [[likely]]case 1:
> > [B4.2] Succs (1): B1
> >
> > [B4]
> > case 1:
> >1: 1
> >2: return [B4.1];
> >Preds (1): B2
> >Succs (1): B0
> >
> > [B0 (EXIT)]
> >Preds (2): B1 B4
> >
> > ```
> > and
> > input:
> >
> > ```
> > int f(int i) {
> > switch (i) {
> > case 1:
> > return 1;
> > }
> > return i;
> > }
> >
> > ```
> > outputs:
> >
> > ```
> > [B4 (ENTRY)]
> >Succs (1): B2
> >
> > [B1]
> >1: i
> >2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
> >3: return [B1.2];
> >Preds (1): B2
> >Succs (1): B0
> >
> > [B2]
> >1: i
> >2: [B2.1] (ImplicitCastExpr, LValueToRValue, int)
> >T: switch [B2.2]
> >Preds (1): B4
> >Succs (2): B3 B1
> >
> > [B3]
> > case 1:
> >1: 1
> >2: return [B3.1];
> >Preds (1): B2
> >Succs (1): B0
> >
> > [B0 (EXIT)]
> >Preds (2): B1 B3
> > ```
> > i think think this is the underlying issue. the false diagnostic from
> > fallthrough previously mentioned is a consequence of this
> Hmm, i see. I got it all wrong. Please forgive me!
>
> You're just trying to make `CFGBuilder` support `AttributedStmt` correctly in
> general. And the logic you're writing says "support it as any other generic
> Stmt that doesn't have any control flow in it, unless it's a
> `[[likely]]`-attributed `AttributedStmt`, in which case jump straight to
> children".
>
> Could you instead do this by implementing `CFGBuilder::VisitAttributedStmt`
> with that logic?
>
> I'm also not sure this logic is actually specific to `[[likely]]`. Maybe we
> should unwrap all `AttributedStmt`s similarly?
we shouldn't handle all Attribu
[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a
Tyker updated this revision to Diff 193269.
Tyker added a comment.
fixed the CFG issue is an proper way
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59467/new/
https://reviews.llvm.org/D59467
Files:
clang/include/clang/AST/Stmt.h
clang/include/clang/AST/StmtCXX.h
clang/include/clang/AST/TextNodeDumper.h
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/AttrDocs.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Scope.h
clang/include/clang/Sema/Sema.h
clang/lib/AST/ASTImporter.cpp
clang/lib/AST/Stmt.cpp
clang/lib/AST/StmtCXX.cpp
clang/lib/AST/TextNodeDumper.cpp
clang/lib/Analysis/CFG.cpp
clang/lib/CodeGen/CGStmt.cpp
clang/lib/CodeGen/CodeGenFunction.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/Parse/ParseStmt.cpp
clang/lib/Sema/Scope.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaStmt.cpp
clang/lib/Sema/SemaStmtAttr.cpp
clang/lib/Sema/TreeTransform.h
clang/lib/Serialization/ASTReaderStmt.cpp
clang/lib/Serialization/ASTWriterStmt.cpp
clang/test/CodeGenCXX/cxx2a-likelihood-attr.cpp
clang/test/PCH/cxx2a-likelihood-attr.cpp
clang/test/SemaCXX/cxx17-compat.cpp
clang/test/SemaCXX/cxx2a-likelihood-attr.cpp
Index: clang/test/SemaCXX/cxx2a-likelihood-attr.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/cxx2a-likelihood-attr.cpp
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -fsyntax-only %s -verify -std=c++2a
+
+// formating this file will break the test
+// clang-format off
+
+int f(int i) {
+ if (i == 1)
+[[unlikely]] { f(i); }
+ else if (i == 2)
+[[likely]] return f(i);
+ else
+[[unlikely]] { return f(i + 1); }
+ return i;
+}
+
+[[likely]] typedef int n1;
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+typedef int [[likely]] n2;
+// expected-error@-1 {{'likely' attribute cannot be applied to types}}
+typedef int n3 [[likely]];
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+
+enum [[likely]] E{One};
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+
+[[likely]]
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+
+void test(int i) {
+ [[likely]]
+ // expected-warning@-1 {{attribute 'likely' is not associated with a branch and is ignored}}
+ if (f(i)) [[likely, likely]] {
+// expected-error@-1 {{likely attribute cannot be repeated}}
+// expected-note@-2 {{previous attribute is here}}
+[[unlikely]] return;
+// expected-warning@-1 {{attribute 'unlikely' is not associated with a branch and is ignored}}
+ }
+ else [[unlikely]] if (f(i)) {
+while (f(i))
+ [[likely]] {
+switch (i) {
+ [[likely]] case 1 : default : [[likely]];
+ // expected-warning@-1 {{attribute 'likely' is not associated with a branch and is ignored}}
+ [[unlikely]] case 3 : f(i);
+ [[fallthrough]];
+case 4:
+ return;
+}
+ }
+for (;;)
+ [[likely, unlikely]]
+ // expected-error@-1 {{unlikely and likely attributes are not compatible}}
+ // expected-note@-2 {{previous attribute is here}}
+ [[likely]] return;
+// expected-error@-1 {{likely attribute cannot be repeated}}
+// expected-note@-5 {{previous attribute is here}}
+ }
+ try { // expected-error {{cannot use 'try' with exceptions disabled}}
+[[likely]];
+// expected-warning@-1 {{attribute 'likely' is not associated with a branch and is ignored}}
+ } catch (int) {
+[[likely]] test :
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+[[unlikely]] return;
+ }
+}
Index: clang/test/SemaCXX/cxx17-compat.cpp
===
--- clang/test/SemaCXX/cxx17-compat.cpp
+++ clang/test/SemaCXX/cxx17-compat.cpp
@@ -63,3 +63,23 @@
// expected-warning@-4 {{range-based for loop initialization statements are incompatible with C++ standards before C++2a}}
#endif
}
+
+//clang-format off
+
+int f(int i) {
+ if (i == 1)
+[[unlikely]]
+#if __cplusplus <= 201703L
+// expected-warning@-2 {{use of the 'unlikely' attribute is a C++2a extension}}
+#endif
+{
+ f(i);
+}
+ else if (i == 2)
+[[likely]]
+#if __cplusplus <= 201703L
+// expected-warning@-2 {{use of the 'likely' attribute is a C++2a extension}}
+#endif
+return f(i);
+ return i;
+}
Index: clang/test/PCH/cxx2a-likelihood-attr.cpp
===
--- /dev/null
+++ clang/test/PCH/cxx2a-likelihood-attr.cpp
@@ -0,0 +1,57 @@
+// Test this without pch.
+// RUN: %clang_cc1 -std=c++2a -fsyntax-only -verify %s -ast-dump | FileCheck %s
+
+// Test with pch. Use '-ast-dump' to force deserialization of function bodies.
+// RUN: %clang_cc1 -x c++-header -std=c++2a -emit-pch -o %t %s
+// RUN: %clang_cc1 -std=c++2a -include-pch %t -fsyntax-only -verify
[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a
Tyker updated this revision to Diff 193284.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59467/new/
https://reviews.llvm.org/D59467
Files:
clang/include/clang/AST/Stmt.h
clang/include/clang/AST/StmtCXX.h
clang/include/clang/AST/TextNodeDumper.h
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/AttrDocs.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Scope.h
clang/include/clang/Sema/Sema.h
clang/lib/AST/ASTImporter.cpp
clang/lib/AST/Stmt.cpp
clang/lib/AST/StmtCXX.cpp
clang/lib/AST/TextNodeDumper.cpp
clang/lib/Analysis/CFG.cpp
clang/lib/CodeGen/CGStmt.cpp
clang/lib/CodeGen/CodeGenFunction.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/Parse/ParseStmt.cpp
clang/lib/Sema/Scope.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaStmt.cpp
clang/lib/Sema/SemaStmtAttr.cpp
clang/lib/Sema/TreeTransform.h
clang/lib/Serialization/ASTReaderStmt.cpp
clang/lib/Serialization/ASTWriterStmt.cpp
clang/test/CodeGenCXX/cxx2a-likelihood-attr.cpp
clang/test/PCH/cxx2a-likelihood-attr.cpp
clang/test/SemaCXX/cxx17-compat.cpp
clang/test/SemaCXX/cxx2a-likelihood-attr.cpp
Index: clang/test/SemaCXX/cxx2a-likelihood-attr.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/cxx2a-likelihood-attr.cpp
@@ -0,0 +1,66 @@
+// RUN: %clang_cc1 -fsyntax-only %s -verify -std=c++2a
+
+// formating this file will break the test
+// clang-format off
+
+int f(int i) {
+ if (i == 1)
+[[unlikely]] { f(i); }
+ else if (i == 2)
+[[likely]] return f(i);
+ else
+[[unlikely]] { return f(i + 1); }
+ return i;
+}
+
+[[likely]] typedef int n1;
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+typedef int [[likely]] n2;
+// expected-error@-1 {{'likely' attribute cannot be applied to types}}
+typedef int n3 [[likely]];
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+
+enum [[likely]] E{One};
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+
+[[likely]]
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+
+void test(int i) {
+ [[likely]]
+ // expected-warning@-1 {{attribute 'likely' is not associated with a branch and is ignored}}
+ if (f(i)) [[likely, likely]] {
+// expected-error@-1 {{likely attribute cannot be repeated}}
+// expected-note@-2 {{previous attribute is here}}
+[[unlikely]] return;
+// expected-warning@-1 {{attribute 'unlikely' is not associated with a branch and is ignored}}
+ }
+ else [[unlikely]] if (f(i)) {
+while (f(i))
+ [[likely]] {
+switch (i) {
+ [[likely]] case 1 : default : [[likely]];
+ // expected-warning@-1 {{attribute 'likely' is not associated with a branch and is ignored}}
+ [[unlikely]] case 3 : f(i);
+ [[fallthrough]];
+case 4:
+ return;
+}
+ }
+for (;;)
+ [[likely, unlikely]]
+ // expected-error@-1 {{unlikely and likely attributes are not compatible}}
+ // expected-note@-2 {{previous attribute is here}}
+ [[likely]] return;
+// expected-error@-1 {{likely attribute cannot be repeated}}
+// expected-note@-5 {{previous attribute is here}}
+ }
+ try { // expected-error {{cannot use 'try' with exceptions disabled}}
+[[likely]];
+// expected-warning@-1 {{attribute 'likely' is not associated with a branch and is ignored}}
+ } catch (int) {
+[[likely]] test :
+// expected-error@-1 {{'likely' attribute cannot be applied to a declaration}}
+[[unlikely]] return;
+ }
+}
Index: clang/test/SemaCXX/cxx17-compat.cpp
===
--- clang/test/SemaCXX/cxx17-compat.cpp
+++ clang/test/SemaCXX/cxx17-compat.cpp
@@ -63,3 +63,23 @@
// expected-warning@-4 {{range-based for loop initialization statements are incompatible with C++ standards before C++2a}}
#endif
}
+
+//clang-format off
+
+int f(int i) {
+ if (i == 1)
+[[unlikely]]
+#if __cplusplus <= 201703L
+// expected-warning@-2 {{use of the 'unlikely' attribute is a C++2a extension}}
+#endif
+{
+ f(i);
+}
+ else if (i == 2)
+[[likely]]
+#if __cplusplus <= 201703L
+// expected-warning@-2 {{use of the 'likely' attribute is a C++2a extension}}
+#endif
+return f(i);
+ return i;
+}
Index: clang/test/PCH/cxx2a-likelihood-attr.cpp
===
--- /dev/null
+++ clang/test/PCH/cxx2a-likelihood-attr.cpp
@@ -0,0 +1,57 @@
+// Test this without pch.
+// RUN: %clang_cc1 -std=c++2a -fsyntax-only -verify %s -ast-dump | FileCheck %s
+
+// Test with pch. Use '-ast-dump' to force deserialization of function bodies.
+// RUN: %clang_cc1 -x c++-header -std=c++2a -emit-pch -o %t %s
+// RUN: %clang_cc1 -std=c++2a -include-pch %t -fsyntax-only -verify %s
+// -ast-dump-all | FileCheck %s
+
+// expected-no-diagnost
[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a
NoQ added inline comments.
Comment at: clang/lib/Analysis/CFG.cpp:2208
+}
+
CFGBlock *CFGBuilder::VisitStmt(Stmt *S, AddStmtChoice asc) {
Tyker wrote:
> NoQ wrote:
> > Tyker wrote:
> > > NoQ wrote:
> > > > Tyker wrote:
> > > > > 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.
> > > > The [[likely]] attribute should not affect the overall topology of the
> > > > CFG. It might be a nice piece of metadata to add to a CFG edge or to a
> > > > CFG terminator, but for most consumers of the CFG (various static
> > > > analyses such as analysis-based warnings or the Static Analyzer) the
> > > > attribute should have little to no effect - the tool would still need
> > > > to explore both branches. I don't know how exactly the fallthrough
> > > > warning operates, but i find it likely (no pun intended) that the
> > > > fallthrough warning itself should be updated, not the CFG.
> > > >
> > > > It is probable that for compiler warnings it'll only cause false
> > > > negatives, which is not as bad as false positives, but i wouldn't rely
> > > > on that. Additionally, false negatives in such rare scenarios will be
> > > > very hard to notice later. So i'm highly in favor of aiming for the
> > > > correct solution in this patch.
> > > >
> > > >
> > > i think we all agree that the CFG structure shouldn't be affected by the
> > > presence or absence of the likely attribute. but in the current state(no
> > > changes to the CFG) it does for example.
> > >
> > > the CFG were obtained without the edit in CFG.cpp but with the added
> > > likely attribute
> > > using: clang -cc1 -analyze test.cpp -analyzer-checker=debug.DumpCFG
> > >
> > > input:
> > >
> > > ```
> > > int f(int i) {
> > > switch (i) {
> > > [[likely]] case 1:
> > > return 1;
> > > }
> > > return i;
> > > }
> > >
> > > ```
> > > outputs:
> > >
> > > ```
> > > [B5 (ENTRY)]
> > >Succs (1): B2
> > >
> > > [B1]
> > >1: i
> > >2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
> > >3: return [B1.2];
> > >Preds (2): B3 B2
> > >Succs (1): B0
> > >
> > > [B2]
> > >1: i
> > >2: [B2.1] (ImplicitCastExpr, LValueToRValue, int)
> > >T: switch [B2.2]
> > >Preds (1): B5
> > >Succs (2): B4 B1
> > >
> > > [B3]
> > >1: [[likely]]case 1:
> > > [B4.2] Succs (1): B1
> > >
> > > [B4]
> > > case 1:
> > >1: 1
> > >2: return [B4.1];
> > >Preds (1): B2
> > >Succs (1): B0
> > >
> > > [B0 (EXIT)]
> > >Preds (2): B1 B4
> > >
> > > ```
> > > and
> > > input:
> > >
> > > ```
> > > int f(int i) {
> > > switch (i) {
> > > case 1:
> > > return 1;
> > > }
> > > return i;
> > > }
> > >
> > > ```
> > > outputs:
> > >
> > > ```
> > > [B4 (ENTRY)]
> > >Succs (1): B2
> > >
> > > [B1]
> > >1: i
> > >2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
> > >3: return [B1.2];
> > >Preds (1): B2
> > >Succs (1): B0
> > >
> > > [B2]
> > >1: i
> > >2: [B2.1] (ImplicitCastExpr, LValueToRValue, int)
> > >T: switch [B2.2]
> > >Preds (1): B4
> > >Succs (2): B3 B1
> > >
> > > [B3]
> > > case 1:
> > >1: 1
> > >2: return [B3.1];
> > >Preds (1): B2
> > >Succs (1): B0
> > >
> > > [B0 (EXIT)]
> > >Preds (2): B1 B3
> > > ```
> > > i think think this is the underlying issue. the false diagnostic from
> > > fallthrough previously mentioned is a consequence of this
> > Hmm, i see. I got it all wrong. Please forgive me!
> >
> > You're just trying to make `CFGBuilder` support `AttributedStmt` correctly
> > in general. And the logic you're writing says "support it as any other
> > generic Stmt that doesn't have any control flow in it, unless it's a
> > `[[likely]]`-attributed `AttributedStmt`, in which case jump straight to
> > c
[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a
martong added a comment. The `ASTImporter.cpp` looks good to me. (Becasue the `BranchHint` is a simple an enum, so we don't need to specifically import that as we would in case of e.g. an `Expr`.) 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
[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a
Tyker reclaimed this revision. Tyker added a comment. I closed it originally because it was inactive and i was working on other reviews. but i learned this is not how it is supposed to be done. 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
