[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D59467#1443173 , @Tyker wrote:

> @lebedev.ri where are tests for AST serialization are located ? i didn't find 
> them.


They live in clang\tests\PCH.

In D59467#1440608 , @Tyker wrote:

> i implemented the semantic the changes for if for, while and do while 
> statement and the AST change to if. can you review it and tell me if it is 
> fine so i implement the rest. i didn't update the test so they will fail.


I think this may be moving closer to the correct direction now, but I'm still 
curious to hear if @rsmith has similar opinions.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8164
+def warn_no_associated_branch : Warning<
+  "attribute %0 not assiciated to any branch is ignored">, 
InGroup;
+def warn_conflicting_attribute : Warning<

Typo: associated

Should probably read "attribute %0 is not associated with a branch and is 
ignored"

Also, I'd rename the diagnostic to be 
`warn_no_likelihood_attr_associated_branch`



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8165-8166
+  "attribute %0 not assiciated to any branch is ignored">, 
InGroup;
+def warn_conflicting_attribute : Warning<
+  "%0 contradicing with previous %1 attribute">, InGroup;
+

I'd rename the diagnostic to `warn_conflicting_likelihood_attrs` and reword the 
diagnostic to "attribute %0 conflicts with previous attribute %1"



Comment at: clang/include/clang/Sema/Scope.h:163
 
+  /// BranchAttr - Likelihood attribute associated with this Branch or nullptr
+  Attr *BranchAttr;

Missing full stop at the end of the comment. You should check all the comments 
in the patch to be sure they are full sentences (start with a capital letter, 
end with punctuation, are grammatically correct, etc).



Comment at: clang/include/clang/Sema/Scope.h:164
+  /// BranchAttr - Likelihood attribute associated with this Branch or nullptr
+  Attr *BranchAttr;
+

Rather than store an `Attr` here, would it make sense to instead store a 
`LikelihoodAttr` directly? That may clean up some casts in the code.



Comment at: clang/include/clang/Sema/Sema.h:3845
+  /// emit diagnostics and determinate the hint for and If statement
+  BranchHint HandleIfStmtHint(Stmt *thenStmt, Stmt *elseStmt, Attr *ThenAttr,
+  Attr *ElseAttr);

You should check all of the identifiers introduced by the patch to ensure they 
meet our coding style requirements 
(https://llvm.org/docs/CodingStandards.html). `thenStmt` -> `ThenStmt`, etc.



Comment at: clang/lib/Sema/SemaStmt.cpp:526
+  Attr *ThenAttr, Attr *ElseAttr) {
+  // diagnose branch with attribute and null statement as empty body
+  if (thenStmt && isa(thenStmt) &&

The condition here is incorrect -- it's not checking what kind of attributed 
statement is present to see if it's a likelihood statement.

However, what is the value in diagnosing this construct? It seems 
not-entirely-unreasonable for a user to write, for instance:
```
if (foo) {
  [[likely]];
  ...
} else [[unlikely]];
```



Comment at: clang/lib/Sema/SemaStmt.cpp:548
+} else {
+  if (ThenAttr->getSpelling()[0] == 'l')
+Hint = Taken;

This is fragile -- you should do something more like:
```
if (const auto *LA = dyn_cast(ThenAttr)) {
  Hint = LA->isLikely() ? ... : ...;
}
```
You'll want to add an accessor onto the LikelihoodAttr object in Attr.td to 
implement the `isLikely()` (and `isUnlikely()`) functions based on the 
attribute spelling. There are examples of this in Attr.td.



Comment at: clang/lib/Sema/SemaStmtAttr.cpp:66-73
+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) {

This doesn't seem to account for `switch` statements, like this, does it?
```
switch (foo) {
[[likely]] case 1: break;
[[unlikely]] case 2: break;
[[likely]] case 3: break;
[[unlikely]] default: break;
}
```
Note, it's perfectly reasonable for there to be repeated attributes here 
because some cases may be more likely than others.


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 Likely Attribute from C++2a to AST

[Gauthier via Phabricator via cfe-commits]

Tyker added a comment.

@lebedev.ri where are tests for AST serialization are located ? i didn't find 
them.


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 Likely Attribute from C++2a to AST

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:1795
 
+  BranchHint getBranchHint() const { return IfStmtBits.Hint; }
+

`lib/AST/TextNodeDumper.cpp` will need to be taught to print it too
(and astdumper test coverage to show that)



Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:225
   bool HasInit = Record.readInt();
+  S->setBranchHint(static_cast(Record.readInt()));
 

[de]serialization code will too need tests
(not just that it is written and read, but that it happens successfully, 
losslessly)


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 Likely Attribute from C++2a to AST

[Gauthier via Phabricator via cfe-commits]

Tyker added a comment.

@riccibruno 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

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

[Gauthier via Phabricator via cfe-commits]

Tyker updated this revision to Diff 192012.

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/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,46 @@
   return ::new (S.Context) auto(Attr);
 }
 
+static Attr *handleLikelihoodAttr(Sema , Stmt *St, const ParsedAttr ,
+  SourceRange Range) {
+  Attr *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();
+
+  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_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 , Stmt *St, const ParsedAttr ,
 SourceRange Range) {
   if (A.getNumArgs() < 1) {
@@ -336,6 +376,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,48 @@
 };
 }
 
-StmtResult
-Sema::ActOnIfStmt(SourceLocation IfLoc, bool IsConstexpr, Stmt *InitStmt,
-  ConditionResult Cond,
-  Stmt *thenStmt, SourceLocation ElseLoc,
-  Stmt *elseStmt) {
+BranchHint Sema::HandleIfStmtHint(Stmt *thenStmt, Stmt *elseStmt,
+  Attr *ThenAttr, Attr *ElseAttr) {
+  // diagnose branch with attribute and null statement as empty body
+  if (thenStmt && isa(thenStmt) &&
+  isa(dyn_cast(thenStmt)->getSubStmt()))
+DiagnoseEmptyStmtBody(
+dyn_cast(thenStmt)->getSubStmt()->getBeginLoc(),
+dyn_cast(thenStmt)->getSubStmt(),
+diag::warn_empty_if_body);
+  if (elseStmt && isa(elseStmt) &&
+  isa(dyn_cast(elseStmt)->getSubStmt()))
+DiagnoseEmptyStmtBody(
+dyn_cast(elseStmt)->getSubStmt()->getBeginLoc(),
+

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

Before any further review, could you please run `clang-format` on your patch 
(but not necessarily on the tests and not on the `*.td` files), wrap lines to 
80 cols, and in general use complete sentences in comments (that is with proper 
punctuation and capitalization) ? To run `clang-format` on your patch you can 
use `clang-format-diff`. This is the command I use :

`git diff -U0 --no-color --no-prefix --cached | clang-format-diff-7 -style=LLVM 
-i -v`, with `--cached` replaced appropriately.


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 Likely Attribute from C++2a to AST

[Gauthier via Phabricator via cfe-commits]

Tyker updated this revision to Diff 192004.
Tyker added a comment.

i implemented the semantic the changes for if for, while and do while statement 
and the AST change to if. can you review it and tell me if it is fine so i 
implement the rest. i didn't update the test so they will fail.


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/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
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGenCXX/cxx2a-likelihood-attr.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,51 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++2a
+
+int f(int i) {
+  if (i == 1) [[unlikely]]
+{
+  return 0;
+}
+  else if (i == 2) [[likely]]
+return 1;
+  return 3;
+}
+
+[[likely]] typedef int n1; // expected-error {{'likely' attribute cannot be applied to a declaration}}
+typedef int [[likely]] n2; // expected-error {{'likely' attribute cannot be applied to types}}
+typedef int n3 [[likely]]; // expected-error {{'likely' attribute cannot be applied to a declaration}}
+
+enum [[likely]] E { // expected-error {{'likely' attribute cannot be applied to a declaration}}
+  One
+};
+
+[[likely]] // expected-error {{'likely' attribute cannot be applied to a declaration}}
+
+void test(int i) {
+  [[likely]] // expected-error {{'likely' can only appear after a selection or iteration statement}}
+if (1) [[likely, likely]] {
+  // expected-warning@-1 {{was already marked likely}}
+  // expected-note@-2 {{previous attribute is here}}
+  [[unlikely]] return ; // expected-error {{'unlikely' can only appear after a selection or iteration statement}}
+}
+  else [[unlikely]] if (1) {
+  while (1) [[likely]] {
+  // switch (i) { switch support isn't implemented yet
+  //   [[likely]] case 1:
+  // default: [[likely]]
+  //   return ;
+  // }
+}
+  for (;;) [[likely, unlikely]]
+  // expected-error@-1 {{unlikely and likely are mutually exclusive}}
+  // expected-note@-2 {{previous attribute is here}}
+[[likely]] return ;
+  // expected-warning@-1 {{was already marked likely}}
+  // expected-note@-5 {{previous attribute is here}}
+}
+  try { // expected-error {{cannot use 'try' with exceptions disabled}}
+[[likely]]; // expected-error {{'likely' can only appear after a selection or iteration statement}}
+  } catch (int) {
+  [[likely]] test: // expected-error {{'likely' attribute cannot be applied to a declaration}}
+  [[unlikely]] return ; // expected-error {{'unlikely' can only appear after a selection or iteration statement}}
+  }
+}
\ No newline at end of file
Index: clang/test/CodeGenCXX/cxx2a-likelihood-attr.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/cxx2a-likelihood-attr.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -std=c++2a -cc1 -emit-llvm -disable-llvm-passes -O3 %s -o - -triple %itanium_abi_triple | FileCheck %s
+
+int test(int i) {
+  if (i == 0) {
+i = i + 1;
+  } else [[likely]]
+return 1;
+  // CHECK: %expval = call i1 @llvm.expect.i1(i1 %cmp, i1 false)
+  while (i == 1) [[unlikely]] {
+return 2;
+  }
+  // CHECK: %[[expval:.*]] = call i1 @llvm.expect.i1(i1 %[[cmp:.*]], i1 false)
+  for (;i == 4;) [[unlikely]] {
+return 2;
+  }
+  // CHECK: %[[expval:.*]] = call i1 @llvm.expect.i1(i1 %[[cmp:.*]], i1 false)
+  do [[likely]] {
+return 2;
+  } while (i == 3);
+  // CHECK: %[[expval:.*]] = call i1 @llvm.expect.i1(i1 %[[cmp:.*]], i1 true)
+  return 0;
+}
Index: clang/test/AST/ast-dump-attr.cpp
===
--- clang/test/AST/ast-dump-attr.cpp
+++ clang/test/AST/ast-dump-attr.cpp
@@ -1,5 +1,39 @@
 // RUN: %clang_cc1 -triple x86_64-pc-linux -std=c++11 -Wno-deprecated-declarations -ast-dump -ast-dump-filter Test %s | FileCheck --strict-whitespace %s
 
+int TestLikelyAttributeIf(int i) {
+  if (i == 1) [[likely]] {
+return 0;
+  } else if (i == 2) [[unlikely]]
+return 1;
+  return 2;
+}
+// CHECK:  IfStmt
+// CHECK:  AttributedStmt
+// CHECK-NEXT: LikelihoodAttr 0x{{[^ ]*}}  likely
+// CHECK:  IfStmt
+// CHECK:  AttributedStmt
+// CHECK-NEXT: LikelihoodAttr 

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

To expand on the above, if you need to add a few bits to a statement/expression 
you can use the bit-field classes in `Stmt` (for example: `IfStmtBitfields`). 
You will also need to update the serialization code in 
`Serialization/ASTWriterStmt.cpp` and `Serialization/ASTReaderStmt.cpp`.


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 Likely Attribute from C++2a to AST

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D59467#1439585 , @Tyker wrote:

> about the semantic issue.
>  with the standard's latitude on where we can put the attribute, the 
> attribute can appear any where on the path of execution and must be matched 
> with the condition it corresponds to. we need to do it for diagnostics 
> purposes to detect conflicts and repetitions. and we need it again for the 
> codegen. but where in the ast can we store this information ?


I feel like this could be tracked in the StmtBits as well as via an attributed 
stmt node. We need to be able to handle cases like:

  if (foo) [[likely]] {
stmts...
  }
  
  if (bar) {
stmts...
[[likely]];
stmts...
  }
  
  if (baz)
[[likely]] stmt;

The attributed statement is required because we want ast pretty printing to 
also print out the `[[likely]]` attribute usage and AST matchers to 
appropriately find the attribute, etc, and we need to know which statement the 
attribute was attached to in order to get the positioning correct. However, for 
diagnostic purposes, we likely want to know information like "has this path 
already been attributed" and that can either be through some extra data on an 
AST node, or it could be an Analysis pass like we do for `[[fallthrough]]` 
support with a separate pass for CodeGen.

> is adding information to conditional statements Node acceptable ?

You can generally add information to AST nodes, but pay close attention to 
things like adding bits to a bit-field that suddenly cause the object size to 
increase and see if there are ways to avoid that if possible (and if not, it's 
still fine).

It might make sense to work your way backwards from the backend to see what 
possible ways we can support this feature. `__builtin_expect` may make sense 
for if statements, but for case and default labels in a switch, we may want to 
play with the branch weights. For catch handlers, we may need to thread 
information through yet another way. Once we know what kinds of paths we can 
actually do something interesting with, we may spot a pattern of how to expose 
the information through the AST.


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 Likely Attribute from C++2a to AST

[Gauthier via Phabricator via cfe-commits]

Tyker added a comment.

about the semantic issue. we will need for diagnostics purposes to detect 
attribute in branches during the semantic phase. so where and how can we store 
this information in the AST so that the CodeGen doesn't have to look through 
branches again for this attributes ?


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 Likely Attribute from C++2a to AST

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In D59467#1439485 , @aaron.ballman 
wrote:

> In D59467#1439473 , @riccibruno 
> wrote:
>
> > Hmm, it seems that an attribute is not allowed by the grammar in the 
> > `expression` or `assignment-expression` of a `conditional-expression`. Was 
> > that intended when `[[likely]]` was added ?
>
>
> Attributes do not appertain to expressions, so yes, that was intentional when 
> we added `likely`. (You can have an attribute applied to an expression 
> statement, but that's at a level that doesn't seem too useful for these 
> attributes.)


Good to know, thanks !


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 Likely Attribute from C++2a to AST

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D59467#1439473 , @riccibruno wrote:

> Hmm, it seems that an attribute is not allowed by the grammar in the 
> `expression` or `assignment-expression` of a `conditional-expression`. Was 
> that intended when `[[likely]]` was added ?


Attributes do not appertain to expressions, so yes, that was intentional when 
we added `likely`. (You can have an attribute applied to an expression 
statement, but that's at a level that doesn't seem too useful for these 
attributes.)


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 Likely Attribute from C++2a to AST

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: rsmith.
aaron.ballman added a comment.

Adding Richard for design strategy discussion.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8164
+def err_must_appear_after_branch : Error<
+  "%0 can only appear after a selection or iteration statement">;
+def warn_attribute_already_present : Warning<

riccibruno wrote:
> I don't think that this is right. I don't see this restriction in the 
> specification. A warning should definitely be emitted when the attribute is 
> ignored, but I believe that an error is inappropriate.
> I don't think that this is right. I don't see this restriction in the 
> specification. A warning should definitely be emitted when the attribute is 
> ignored, but I believe that an error is inappropriate.

Agreed.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8165
+  "%0 can only appear after a selection or iteration statement">;
+def warn_attribute_already_present : Warning<
+  "was already marked %0">;

This diagnostic isn't needed -- `warn_duplicate_attribute_exact` will cover it.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8167
+  "was already marked %0">;
+def err_mutuably_exclusive_likelihood : Error<
+  "%0 and %1 are mutually exclusive">;

This diagnostic is not needed -- `err_attributes_are_not_compatible` will cover 
it.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8170
+def err_contradictory_attribute : Warning<
+  "%0 contradicing with previous attribute">;
+

riccibruno wrote:
> This should be `warn_contradictory_attribute`, and I am not seeing tests for 
> this.
This note also isn't needed. You should use `note_conflicting_attribute` 
instead.



Comment at: clang/lib/Sema/SemaStmtAttr.cpp:64-75
+  if (CurScope) {
+Scope* ControlScope = CurScope->getParent();
+if (!ControlScope ||
+!(ControlScope->getFlags() & Scope::ControlScope ||
+  ControlScope->getFlags() & Scope::BreakScope) ||
+CurScope->getFlags() & Scope::CompoundStmtScope ||
+ ControlScope->getFlags() & Scope::SEHExceptScope ||

This is incorrect -- nothing in the standard requires the attribute to appear 
after a branch statement. I'm not even 100% convinced that this should map 
directly to `__builtin_expect` in all cases (though it certainly would for 
conditional branches), because that doesn't help with things like catch 
handlers or labels.



Comment at: clang/test/SemaCXX/cxx2a-likelihood-attr.cpp:52
+}
\ No newline at end of file


Be sure to add the newline, 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 Likely Attribute from C++2a to AST

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

Hmm, it seems that an attribute is not allowed by the grammar in the 
`expression` or `assignment-expression` of a `conditional-expression`. Was that 
intended when `[[likely]]` was added ?


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 Likely Attribute from C++2a to AST

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: clang/lib/Sema/SemaStmtAttr.cpp:74
+ ControlScope->getFlags() & Scope::FnTryCatchScope)
+  S.Diag(A.getLoc(), diag::err_must_appear_after_branch) << A.getName();
+  }

I think that you need to test for each of the above.


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 Likely Attribute from C++2a to AST

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

Additionally I think that you need to:

- add tests for templates
- handle the ternary operator

Also, as per the coding guidelines, you need to fix:

- spelling of variables, eg `hint` -> `Hint`.
- run `clang-format` on your patch.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8164
+def err_must_appear_after_branch : Error<
+  "%0 can only appear after a selection or iteration statement">;
+def warn_attribute_already_present : Warning<

I don't think that this is right. I don't see this restriction in the 
specification. A warning should definitely be emitted when the attribute is 
ignored, but I believe that an error is inappropriate.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8170
+def err_contradictory_attribute : Warning<
+  "%0 contradicing with previous attribute">;
+

This should be `warn_contradictory_attribute`, and I am not seeing tests for 
this.



Comment at: clang/lib/CodeGen/CodeGenFunction.h:4022
+
+  static BranchHint getBranchHint(const Stmt* S, bool Invert = false);
+

Doc ?



Comment at: clang/lib/Parse/ParseStmt.cpp:1309
+}
+
 // Pop the 'else' scope if needed.

I don't think that this should be done here. The natural place to me seems to 
be in `ActOnIfStmt`. Additionally I think that you should wrap this in a static 
helper function.



Comment at: clang/lib/Sema/SemaStmtAttr.cpp:232
   for (const auto *I : Attrs) {
+
+if (isa(I)) {

A comment indicating what this is doing ?


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 Likely Attribute from C++2a to AST

[Gauthier via Phabricator via cfe-commits]

Tyker updated this revision to Diff 191862.
Tyker added a comment.

handled codegen for if, while, for and do/while, it will generate a 
@llvm.expect before the condition based on the attribute
i changed slithly the semantic

  if (...) {  //error
  [[likely]] ...
  }
  
  if (...) [[likely]]  { // ok
   ...
  }

and added tests for AST, Semantic and codegen


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

https://reviews.llvm.org/D59467

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/CodeGenCXX/cxx2a-likelihood-attr.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,51 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++2a
+
+int f(int i) {
+  if (i == 1) [[unlikely]]
+{
+  return 0;
+}
+  else if (i == 2) [[likely]]
+return 1;
+  return 3;
+}
+
+[[likely]] typedef int n1; // expected-error {{'likely' attribute cannot be applied to a declaration}}
+typedef int [[likely]] n2; // expected-error {{'likely' attribute cannot be applied to types}}
+typedef int n3 [[likely]]; // expected-error {{'likely' attribute cannot be applied to a declaration}}
+
+enum [[likely]] E { // expected-error {{'likely' attribute cannot be applied to a declaration}}
+  One
+};
+
+[[likely]] // expected-error {{'likely' attribute cannot be applied to a declaration}}
+
+void test(int i) {
+  [[likely]] // expected-error {{'likely' can only appear after a selection or iteration statement}}
+if (1) [[likely, likely]] {
+  // expected-warning@-1 {{was already marked likely}}
+  // expected-note@-2 {{previous attribute is here}}
+  [[unlikely]] return ; // expected-error {{'unlikely' can only appear after a selection or iteration statement}}
+}
+  else [[unlikely]] if (1) {
+  while (1) [[likely]] {
+  // switch (i) { switch support isn't implemented yet
+  //   [[likely]] case 1:
+  // default: [[likely]]
+  //   return ;
+  // }
+}
+  for (;;) [[likely, unlikely]]
+  // expected-error@-1 {{unlikely and likely are mutually exclusive}}
+  // expected-note@-2 {{previous attribute is here}}
+[[likely]] return ;
+  // expected-warning@-1 {{was already marked likely}}
+  // expected-note@-5 {{previous attribute is here}}
+}
+  try { // expected-error {{cannot use 'try' with exceptions disabled}}
+[[likely]]; // expected-error {{'likely' can only appear after a selection or iteration statement}}
+  } catch (int) {
+  [[likely]] test: // expected-error {{'likely' attribute cannot be applied to a declaration}}
+  [[unlikely]] return ; // expected-error {{'unlikely' can only appear after a selection or iteration statement}}
+  }
+}
\ No newline at end of file
Index: clang/test/CodeGenCXX/cxx2a-likelihood-attr.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/cxx2a-likelihood-attr.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -std=c++2a -cc1 -emit-llvm -disable-llvm-passes -O3 %s -o - -triple %itanium_abi_triple | FileCheck %s
+
+int test(int i) {
+  if (i == 0) {
+i = i + 1;
+  } else [[likely]]
+return 1;
+  // CHECK: %expval = call i1 @llvm.expect.i1(i1 %cmp, i1 false)
+  while (i == 1) [[unlikely]] {
+return 2;
+  }
+  // CHECK: %[[expval:.*]] = call i1 @llvm.expect.i1(i1 %[[cmp:.*]], i1 false)
+  for (;i == 4;) [[unlikely]] {
+return 2;
+  }
+  // CHECK: %[[expval:.*]] = call i1 @llvm.expect.i1(i1 %[[cmp:.*]], i1 false)
+  do [[likely]] {
+return 2;
+  } while (i == 3);
+  // CHECK: %[[expval:.*]] = call i1 @llvm.expect.i1(i1 %[[cmp:.*]], i1 true)
+  return 0;
+}
Index: clang/test/AST/ast-dump-attr.cpp
===
--- clang/test/AST/ast-dump-attr.cpp
+++ clang/test/AST/ast-dump-attr.cpp
@@ -1,5 +1,39 @@
 // RUN: %clang_cc1 -triple x86_64-pc-linux -std=c++11 -Wno-deprecated-declarations -ast-dump -ast-dump-filter Test %s | FileCheck --strict-whitespace %s
 
+int TestLikelyAttributeIf(int i) {
+  if (i == 1) [[likely]] {
+return 0;
+  } else if (i == 2) [[unlikely]]
+return 1;
+  return 2;
+}
+// CHECK:  IfStmt
+// CHECK:  AttributedStmt
+// CHECK-NEXT: LikelihoodAttr 0x{{[^ ]*}}  likely
+// CHECK:  IfStmt
+// CHECK:  AttributedStmt
+// CHECK-NEXT: LikelihoodAttr 0x{{[^ ]*}}  unlikely
+
+int TestLikelyAttributeLoops(int i) {
+  while (i == 1) [[likely]] {
+return 0;
+  }
+  for (;;) [[unlikely]]
+do [[likely]] {
+  return 1;
+} while (i == 2);
+  

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

[S. B. Tam via Phabricator via cfe-commits]

cpplearner added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8166
+def err_multiple_likelihood : Error<
+  "there can only be one %0 attribue in any attribute list">;
+def err_mutuably_exclusive_likelihood : Error<

attribue => attribute



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8172
+def note_previous_likelihood : Note<
+  "previously used %0 attribue">;
+

attribue => attribute



Comment at: clang/lib/Parse/ParseStmt.cpp:1329
+  } else if (ElseBranchAttr) {
+if (ThenBranchAttr->getSpelling()[0] == 'l')
+  WhichBranch = IfStmt::ElseBranch;

Should this use `ElseBranchAttr` instead of `ThenBranchAttr`?


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 Likely Attribute from C++2a to AST

[Gauthier via Phabricator via cfe-commits]

Tyker updated this revision to Diff 191408.
Tyker added a comment.
Herald added a subscriber: jdoerfert.

added diagnostics for contradictory attributes like for if:

  if (...) [[likely]]
return 1;
  else [[likely]]
return 2;

handled the codegen for If to generate builtin_expect but i probably didn't do 
it the canonical way. i am awaiting advice of how to do it right.


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/ScopeInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.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,51 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++2a
+
+int f(int i) {
+  if (i == 1) [[unlikely]]
+{
+  return 0;
+}
+  else if (i == 2) [[likely]]
+return 1;
+  return 3;
+}
+
+[[likely]] typedef int n1; // expected-error {{'likely' attribute cannot be applied to a declaration}}
+typedef int [[likely]] n2; // expected-error {{'likely' attribute cannot be applied to types}}
+typedef int n3 [[likely]]; // expected-error {{'likely' attribute cannot be applied to a declaration}}
+
+enum [[likely]] E { // expected-error {{'likely' attribute cannot be applied to a declaration}}
+  One
+};
+
+[[likely]] // expected-error {{'likely' attribute cannot be applied to a declaration}}
+
+void test(int i) {
+  [[likely]] // expected-error {{'likely' attribute can only appear in if, while, switch and for}}
+if (1) [[likely, likely]] {
+  // expected-error@-1 {{there can only be one likely attribue in any attribute list}}
+  // expected-note@-2 {{previously used likely attribue}}
+  [[unlikely]] return ; // expected-error {{'unlikely' attribute can only appear in if, while, switch and for}}
+}
+  else [[unlikely]] if (1) {
+  while (1) [[likely]] {
+  switch (i) {
+[[likely]] case 1:
+  default: [[likely]]
+return ;
+  }
+}
+  for (;;) [[likely, unlikely]]
+  // expected-error@-1 {{unlikely and likely are mutually exclusive}}
+  // expected-note@-2 {{previously used likely attribue}}
+[[likely]] return ;
+  // expected-error@-1 {{likely and unlikely are mutually exclusive}}
+  // expected-note@-5 {{previously used unlikely attribue}}
+}
+  try { // expected-error {{cannot use 'try' with exceptions disabled}}
+[[likely]]; // expected-error {{'likely' attribute can only appear in if, while, switch and for}}
+  } catch (int) {
+  [[likely]] test: // expected-error {{'likely' attribute cannot be applied to a declaration}}
+  [[unlikely]] return ;
+  }
+}
\ No newline at end of file
Index: clang/lib/Sema/SemaStmtAttr.cpp
===
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -51,6 +51,50 @@
   return ::new (S.Context) auto(Attr);
 }
 
+static Attr *handleLikelihoodAttr(Sema , Stmt *St, const ParsedAttr ,
+   SourceRange Range) {
+  LikelihoodAttr Attribute(A.getRange(), S.Context,
+   A.getAttributeSpellingListIndex());
+
+  Scope* scope = S.getCurScope();
+  Scope* previousScope = nullptr;
+
+  if (scope)
+previousScope = scope->getParent();
+
+  //check that ths attribute is used in an if, while, for, switch or catch
+  if (!previousScope || 
+  !(previousScope->getFlags() & Scope::ControlScope) ||
+   previousScope->getFlags() & Scope::SEHExceptScope ||
+   previousScope->getFlags() & Scope::SEHTryScope ||
+   previousScope->getFlags() & Scope::FnTryCatchScope)
+ S.Diag(A.getLoc(), diag::err_likelihood_outside_control_scope) << A.getName();
+
+  if (!S.getLangOpts().CPlusPlus2a)
+S.Diag(A.getLoc(), diag::ext_cxx2a_attr) << A.getName();
+
+  clang::Attr* result = ::new (S.Context) auto(Attribute);
+
+  auto* FnScope = S.getCurFunction();
+
+  /// don't need to emit any diagnostics if we aren't in a function or the stack is empty as this is already covered by the checks above
+  if (FnScope && !FnScope->PathLikelyhoodAttrStack.empty()) {
+Attr*& TopStackAttr = FnScope->PathLikelyhoodAttrStack.back();
+if (TopStackAttr) {
+  /// only need to compare first character to differenciate between 'likely' and 'unlikely'
+  if (result->getSpelling()[0] == TopStackAttr->getSpelling()[0])
+S.Diag(result->getLocation(), 

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

[Gauthier via Phabricator via cfe-commits]

Tyker updated this revision to Diff 191115.
Tyker added a comment.

i think we are suppose to hook likely/unlikely on builtin_expected, for if, 
for, while, switch. but i have no idea how we could hook it if we need to 
support catch.

i added a revision with likely/unlikely and the correct semantic (i think).

i also added the required checks. i didn't find a way to detect when we are in 
a catch block. but it should perhaps be allowded.


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

https://reviews.llvm.org/D59467

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaStmtAttr.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,51 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++2a
+
+int f(int i) {
+  if (i == 1) [[unlikely]]
+{
+  return 0;
+}
+  else if (i == 2) [[likely]]
+return 1;
+  return 3;
+}
+
+[[likely]] typedef int n1; // expected-error {{'likely' attribute cannot be applied to a declaration}}
+typedef int [[likely]] n2; // expected-error {{'likely' attribute cannot be applied to types}}
+typedef int n3 [[likely]]; // expected-error {{'likely' attribute cannot be applied to a declaration}}
+
+enum [[likely]] E { // expected-error {{'likely' attribute cannot be applied to a declaration}}
+  One
+};
+
+[[likely]] // expected-error {{'likely' attribute cannot be applied to a declaration}}
+
+void test(int i) {
+  [[likely]] // expected-error {{'likely' attribute can only appear in if, while, switch and for}}
+if (1) [[likely, likely]] {
+  // expected-error@-1 {{there can only be one likely attribue in any attribute list}}
+  // expected-note@-2 {{previously used likely attribue}}
+  [[unlikely]] return ; // expected-error {{'unlikely' attribute can only appear in if, while, switch and for}}
+}
+  else [[unlikely]] if (1) {
+  while (1) [[likely]] {
+  switch (i) {
+[[likely]] case 1:
+  default: [[likely]]
+return ;
+  }
+}
+  for (;;) [[likely, unlikely]]
+  // expected-error@-1 {{unlikely and likely are mutually exclusive}}
+  // expected-note@-2 {{previously used likely attribue}}
+[[likely]] return ;
+  // expected-error@-1 {{likely and unlikely are mutually exclusive}}
+  // expected-note@-5 {{previously used unlikely attribue}}
+}
+  try { // expected-error {{cannot use 'try' with exceptions disabled}}
+[[likely]]; // expected-error {{'likely' attribute can only appear in if, while, switch and for}}
+  } catch (int) {
+  [[likely]] test: // expected-error {{'likely' attribute cannot be applied to a declaration}}
+  [[unlikely]] return ;
+  }
+}
\ No newline at end of file
Index: clang/lib/Sema/SemaStmtAttr.cpp
===
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -51,6 +51,31 @@
   return ::new (S.Context) auto(Attr);
 }
 
+static Attr *handleLikelihoodAttr(Sema , Stmt *St, const ParsedAttr ,
+   SourceRange Range) {
+  LikelihoodAttr Attr(A.getRange(), S.Context,
+   A.getAttributeSpellingListIndex());
+
+  Scope* scope = S.getCurScope();
+  Scope* previousScope = nullptr;
+
+  if (scope)
+previousScope = scope->getParent();
+
+  //check that ths attribute is used in an if, while, for, switch or catch
+  if (!previousScope || 
+  !(previousScope->getFlags() & Scope::ControlScope) ||
+   previousScope->getFlags() & Scope::SEHExceptScope ||
+   previousScope->getFlags() & Scope::SEHTryScope ||
+   previousScope->getFlags() & Scope::FnTryCatchScope)
+ S.Diag(A.getLoc(), diag::err_likelihood_outside_control_scope) << A.getName();
+
+  if (!S.getLangOpts().CPlusPlus2a)
+S.Diag(A.getLoc(), diag::ext_cxx2a_attr) << A.getName();
+
+  return ::new (S.Context) auto(Attr);
+}
+
 static Attr *handleSuppressAttr(Sema , Stmt *St, const ParsedAttr ,
 SourceRange Range) {
   if (A.getNumArgs() < 1) {
@@ -201,7 +226,21 @@
   } HintAttrs[] = {{nullptr, nullptr}, {nullptr, nullptr}, {nullptr, nullptr},
{nullptr, nullptr}, {nullptr, nullptr}, {nullptr, nullptr}};
 
+  //there can be only one likelyhood attribute
+  const Attr* likelihoodAttr = nullptr;
+
   for (const auto *I : Attrs) {
+if (llvm::isa(I)) {
+  if (likelihoodAttr) {
+if (std::strcmp(I->getSpelling(), likelihoodAttr->getSpelling()))
+  S.Diag(I->getLocation(), diag::err_mutuably_exclusive_likelihood) << I->getSpelling() << likelihoodAttr->getSpelling();
+else
+  S.Diag(I->getLocation(), diag::err_multiple_likelihood) << I->getSpelling();
+

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D59467#1432675 , @Tyker wrote:

> if likely/unlikely can be applied to any statement or label what should be 
> generated when it is applied to a statement that don't have any conditional 
> branch ? should it be ignored  ?


That's the semantics we need to figure out. The standard talks about paths of 
execution including the attribute 
(http://eel.is/c++draft/dcl.attr.likelihood#2.sentence-1), so I imagine this 
means we should be supporting code like:

  void func(bool some_condition) {
if (some_condition) {
  [[likely]];
} else {
  [[unlikely]];
}
  }

While that may seem a bit silly (because you can add the attribute to the 
compound-statement), there are situations where you cannot do that, but the 
feature still has use. Consider:

  void func() {
try {
  some_operation();
} catch (some_exception ) {
  [[likely]];
  recover_without_killing_everyone_on_the_elevator();
}
  }

In some safety-critical situations, you may not care how fast the normal 
operation works but still need the failure mode to be highly optimized to avoid 
loss of life.

We have other fun questions to figure out as well, such as whether to diagnose 
this code and what the semantics should be:

  void func(bool foo) {
if (foo) {
  [[likely]];
  [[unlikely]];
}
  }
  
  void func2(bool foo) {
if (foo) {
  [[likely]];
} else {
  [[likely]];
}
  }

We don't have to solve it all up front, but having an idea as to what the 
backend is going to do with this information may help us design the appropriate 
frontend behavior where the standard gives us this much latitude. Do you know 
how this is expected to hook in to LLVM?


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 Likely Attribute from C++2a to AST

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D59467#1432675 , @Tyker wrote:

> if likely/unlikely can be applied to any statement or label what should be 
> generated when it is applied to a statement that don't have any conditional 
> branch ? should it be ignored without warning or error ?


Disregard previous [deleted] comment from me.
Yes, i believe it should simply be always accepted, for any 
`isa()`.
How to model that in codegen i'm not sure yet. Likely by affecting the nearest 
(up the AST) branch / switch / etc.


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 Likely Attribute from C++2a to AST

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D59467#1432675 , @Tyker wrote:

> if likely/unlikely can be applied to any statement or label what should be 
> generated when it is applied to a statement that don't have any conditional 
> branch ? should it be ignored without warning or error ?


I think this should be modelled as a new statement type, `AttributeDeclStmt` or 
even `LikelihoodDeclStmt`.
I'm not fully sure how to model it in codegen.


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 Likely Attribute from C++2a to AST

[Gauthier via Phabricator via cfe-commits]

Tyker added a comment.

if likely/unlikely can be applied to any statement or label what should be 
generated when it is applied to a statement that don't have any conditional 
branch ? should it be ignored without warning or error ?


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 Likely Attribute from C++2a to AST

[Gauthier via Phabricator via cfe-commits]

Tyker marked an inline comment as done.
Tyker added a comment.

I added the sema testes


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 Likely Attribute from C++2a to AST

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1151
+def Likely : StmtAttr {
+  let Spellings = [CXX11<"", "likely", 201903>, CXX11<"clang", "likely">];
+  let Documentation = [LikelyDocs];

I think this should have a C spelling as well, so I'd probably go with: 
`[CXX11<"", "likely", 201903L>, Clang<"likely">]` and then similar for 
`unlikely`. I would probably use the same semantic attribute for both (so add 
both spellings and an accessor to differentiate). Rather than naming the 
attribute `Likely`, I would go with `Likelihood` so it covers both spellings 
more naturally.



Comment at: clang/include/clang/Basic/Attr.td:1153
+  let Documentation = [LikelyDocs];
+}
+

Can you add a commented-out `Subjects` that take `Stmt` and `LabelStmt`? It's 
not useful now, but I still hope that someday we can tablegen more checks for 
statement and type attributes like we already do for declarations.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8164
+def err_likely_attr_invalid_placement : Error<
+  "likely annotation can't apear here">;
+

lebedev.ri wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > appear
> > Please mark fixed comments as fixed (checkbox).
> 'here'?
> Would be nicer to be more explanatory.
> `likely annotation can't appear on %0; can only appear on x, y, x`
This should follow the pattern used by decl attributes:
```
def err_attribute_wrong_stmt_type : Error<
  "%0 attribute only applies to statements and labels">;
```
There may be a diagnostic used for `[[fallthrough]]` that could be combined 
with this diagnostic, since that attribute can only be applied to null 
statements.


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 Likely Attribute from C++2a to AST

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I believe this has the incorrect modeling -- in the following code example, the 
attribute appertains to the substatement of the if statement, not the if 
statement itself:

  if (foo) [[likely]] {
blah;
  }

This falls out from:  http://eel.is/c++draft/stmt.stmt#1. You model this by 
applying the attribute to the `if` statement, which will have the wrong 
semantics when you hook up the hints to the backend.




Comment at: clang/test/SemaCXX/cxx2a-likely-attr.cpp:27
+  switch (n) {
+[[likely]] return; // expected-error {{likely annotation can't appear 
here}}
+

Why? The attribute appertains to statements, and `return` is a statement. This 
should be accepted, I believe.



Comment at: clang/test/SemaCXX/cxx2a-likely-attr.cpp:29
+
+  case 0:
+if (1) [[likely, likely]] // expected-error {{attribute 'likely' cannot 
appear multiple times in an attribute specifier}}

Missing a test case that the attribute should apply to labels like this (should 
also have a test for non-switch labels as well).



Comment at: clang/test/SemaCXX/cxx2a-likely-attr.cpp:30
+  case 0:
+if (1) [[likely, likely]] // expected-error {{attribute 'likely' cannot 
appear multiple times in an attribute specifier}}
+  return ;

Also missing the test case covering 
http://eel.is/c++draft/dcl.attr.likelihood#1.sentence-3


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 Likely Attribute from C++2a to AST

[Gauthier via Phabricator via cfe-commits]

Tyker updated this revision to Diff 191035.

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

https://reviews.llvm.org/D59467

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/AST/ast-dump-attr.cpp
  clang/test/SemaCXX/cxx2a-likely-attr.cpp

Index: clang/test/SemaCXX/cxx2a-likely-attr.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/cxx2a-likely-attr.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++2a
+
+int f(int i) {
+  if (i == 1)
+  {
+  return 0;
+}
+  else if (i == 2) [[likely]]
+return 1;
+  return 3;
+}
+
+[[likely]] typedef int n1; // expected-error {{'likely' attribute cannot be applied to a declaration}}
+typedef int [[likely]] n2; // expected-error {{'likely' attribute cannot be applied to types}}
+typedef int n3 [[likely]]; // expected-error {{'likely' attribute cannot be applied to a declaration}}
+
+enum [[likely]] E { // expected-error {{'likely' attribute cannot be applied to a declaration}}
+  One
+};
+
+[[likely]] // expected-error {{'likely' attribute cannot be applied to a declaration}}
+void g(void) {
+  [[likely]] int n; // expected-error {{'likely' attribute cannot be applied to a declaration}}
+  [[likely]] ++n; // expected-error {{likely annotation can't appear here}}
+  
+  switch (n) {
+[[likely]] return; // expected-error {{likely annotation can't appear here}}
+
+  case 0:
+if (1) [[likely, likely]] // expected-error {{attribute 'likely' cannot appear multiple times in an attribute specifier}}
+  return ;
+if (1) [[likely(0)]] // expected-error {{attribute 'likely' cannot have an argument list}}
+  return ;
+break;
+  }
+}
+
Index: clang/test/AST/ast-dump-attr.cpp
===
--- clang/test/AST/ast-dump-attr.cpp
+++ clang/test/AST/ast-dump-attr.cpp
@@ -23,6 +23,21 @@
 // CHECK-NEXT:   FallThroughAttr
 // CHECK-NEXT:   NullStmt
 
+int TestLikelyAttribute(int i) {
+  if (i == 1) [[likely]] {
+return 0;
+  } else if (i == 2) [[likely]]
+return 1;
+  return 2;
+}
+// CHECK:  FunctionDecl{{.*}}TestLikelyAttribute
+// CHECK:  AttributedStmt
+// CHECK-NEXT: LikelyAttr
+// CHECK-NEXT: IfStmt
+// CHECK:  AttributedStmt
+// CHECK-NEXT: LikelyAttr
+// CHECK-NEXT: IfStmt
+
 [[clang::warn_unused_result]] int TestCXX11DeclAttr();
 // CHECK:  FunctionDecl{{.*}}TestCXX11DeclAttr
 // CHECK-NEXT:   WarnUnusedResultAttr
Index: clang/lib/Sema/SemaStmtAttr.cpp
===
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -51,6 +51,21 @@
   return ::new (S.Context) auto(Attr);
 }
 
+static Attr *handleLikelyAttr(Sema , Stmt *St, const ParsedAttr ,
+   SourceRange Range) {
+  LikelyAttr Attr(A.getRange(), S.Context,
+   A.getAttributeSpellingListIndex());
+
+  if (!llvm::isa(St)) {
+S.Diag(A.getLoc(), diag::err_likely_attr_invalid_placement) << A.getName();
+  }
+
+  if (!S.getLangOpts().CPlusPlus2a && !A.getScopeName())
+S.Diag(A.getLoc(), diag::ext_cxx2a_attr) << A.getName();
+
+  return ::new (S.Context) auto(Attr);
+}
+
 static Attr *handleSuppressAttr(Sema , Stmt *St, const ParsedAttr ,
 SourceRange Range) {
   if (A.getNumArgs() < 1) {
@@ -336,6 +351,8 @@
 return nullptr;
   case ParsedAttr::AT_FallThrough:
 return handleFallThroughAttr(S, St, A, Range);
+  case ParsedAttr::AT_Likely:
+return handleLikelyAttr(S, St, A, Range);
   case ParsedAttr::AT_LoopHint:
 return handleLoopHintAttr(S, St, A, Range);
   case ParsedAttr::AT_OpenCLUnrollHint:
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -1216,6 +1216,9 @@
 : Sema::ConditionKind::Boolean))
 return StmtError();
 
+  ParsedAttributesWithRange Attrs(AttrFactory);
+  MaybeParseCXX11Attributes(Attrs);
+
   llvm::Optional ConstexprCondition;
   if (IsConstexpr)
 ConstexprCondition = Cond.getKnownValue();
@@ -1314,8 +1317,13 @@
   if (ElseStmt.isInvalid())
 ElseStmt = Actions.ActOnNullStmt(ElseStmtLoc);
 
-  return Actions.ActOnIfStmt(IfLoc, IsConstexpr, InitStmt.get(), Cond,
- ThenStmt.get(), ElseLoc, ElseStmt.get());
+  StmtResult Stmt = Actions.ActOnIfStmt(IfLoc, IsConstexpr, InitStmt.get(), Cond,
+ ThenStmt.get(), ElseLoc, ElseStmt.get());
+
+  if (!Attrs.empty())
+return Actions.ProcessStmtAttributes(Stmt.get(), Attrs, Attrs.Range);
+
+  return Stmt;
 }
 
 /// ParseSwitchStatement
Index: 

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D59467#1432587 , @Tyker wrote:

> i added tests as you requested. i didn't add test for the codegen as it 
> wasn't affected.


Ah right, there wasn't some other clang-specific spelling for this already it 
seems, so indeed codegen needs wiring up too.

Sema tests still missing. (correct usage of attribute in various places with 
different `-std=?` params, incorrect usage of attribute in incorrect places, 
etc)




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8164
+def err_likely_attr_invalid_placement : Error<
+  "likely annotation can't apear here">;
+

lebedev.ri wrote:
> appear
Please mark fixed comments as fixed (checkbox).



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8164
+def err_likely_attr_invalid_placement : Error<
+  "likely annotation can't apear here">;
+

lebedev.ri wrote:
> lebedev.ri wrote:
> > appear
> Please mark fixed comments as fixed (checkbox).
'here'?
Would be nicer to be more explanatory.
`likely annotation can't appear on %0; can only appear on x, y, x`



Comment at: clang/lib/Sema/SemaStmtAttr.cpp:59
+
+  if (!llvm::isa(St)) {
+S.Diag(A.getLoc(), diag::err_likely_attr_invalid_placement) << A.getName();

Where else can it appear?


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 Likely Attribute from C++2a to AST

[Gauthier via Phabricator via cfe-commits]

Tyker updated this revision to Diff 191034.

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

https://reviews.llvm.org/D59467

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/AST/ast-dump-attr.cpp

Index: clang/test/AST/ast-dump-attr.cpp
===
--- clang/test/AST/ast-dump-attr.cpp
+++ clang/test/AST/ast-dump-attr.cpp
@@ -23,6 +23,21 @@
 // CHECK-NEXT:   FallThroughAttr
 // CHECK-NEXT:   NullStmt
 
+int TestLikelyAttribute(int i) {
+  if (i == 1) [[likely]] {
+return 0;
+  } else if (i == 2) [[likely]]
+return 1;
+  return 2;
+}
+// CHECK:  FunctionDecl{{.*}}TestLikelyAttribute
+// CHECK:  AttributedStmt
+// CHECK-NEXT: LikelyAttr
+// CHECK-NEXT: IfStmt
+// CHECK:  AttributedStmt
+// CHECK-NEXT: LikelyAttr
+// CHECK-NEXT: IfStmt
+
 [[clang::warn_unused_result]] int TestCXX11DeclAttr();
 // CHECK:  FunctionDecl{{.*}}TestCXX11DeclAttr
 // CHECK-NEXT:   WarnUnusedResultAttr
Index: clang/lib/Sema/SemaStmtAttr.cpp
===
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -51,6 +51,21 @@
   return ::new (S.Context) auto(Attr);
 }
 
+static Attr *handleLikelyAttr(Sema , Stmt *St, const ParsedAttr ,
+   SourceRange Range) {
+  LikelyAttr Attr(A.getRange(), S.Context,
+   A.getAttributeSpellingListIndex());
+
+  if (!llvm::isa(St)) {
+S.Diag(A.getLoc(), diag::err_likely_attr_invalid_placement) << A.getName();
+  }
+
+  if (!S.getLangOpts().CPlusPlus2a && !A.getScopeName())
+S.Diag(A.getLoc(), diag::ext_cxx2a_attr) << A.getName();
+
+  return ::new (S.Context) auto(Attr);
+}
+
 static Attr *handleSuppressAttr(Sema , Stmt *St, const ParsedAttr ,
 SourceRange Range) {
   if (A.getNumArgs() < 1) {
@@ -336,6 +351,8 @@
 return nullptr;
   case ParsedAttr::AT_FallThrough:
 return handleFallThroughAttr(S, St, A, Range);
+  case ParsedAttr::AT_Likely:
+return handleLikelyAttr(S, St, A, Range);
   case ParsedAttr::AT_LoopHint:
 return handleLoopHintAttr(S, St, A, Range);
   case ParsedAttr::AT_OpenCLUnrollHint:
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -1216,6 +1216,9 @@
 : Sema::ConditionKind::Boolean))
 return StmtError();
 
+  ParsedAttributesWithRange Attrs(AttrFactory);
+  MaybeParseCXX11Attributes(Attrs);
+
   llvm::Optional ConstexprCondition;
   if (IsConstexpr)
 ConstexprCondition = Cond.getKnownValue();
@@ -1314,8 +1317,13 @@
   if (ElseStmt.isInvalid())
 ElseStmt = Actions.ActOnNullStmt(ElseStmtLoc);
 
-  return Actions.ActOnIfStmt(IfLoc, IsConstexpr, InitStmt.get(), Cond,
- ThenStmt.get(), ElseLoc, ElseStmt.get());
+  StmtResult Stmt = Actions.ActOnIfStmt(IfLoc, IsConstexpr, InitStmt.get(), Cond,
+ ThenStmt.get(), ElseLoc, ElseStmt.get());
+
+  if (!Attrs.empty())
+return Actions.ProcessStmtAttributes(Stmt.get(), Attrs, Attrs.Range);
+
+  return Stmt;
 }
 
 /// ParseSwitchStatement
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -3858,6 +3858,7 @@
   case ParsedAttr::AT_Deprecated:
   case ParsedAttr::AT_FallThrough:
   case ParsedAttr::AT_CXX11NoReturn:
+  case ParsedAttr::AT_Likely:
 return true;
   case ParsedAttr::AT_WarnUnusedResult:
 return !ScopeName && AttrName->getName().equals("nodiscard");
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7339,6 +7339,8 @@
   "use of the %0 attribute is a C++14 extension">, InGroup;
 def ext_cxx17_attr : Extension<
   "use of the %0 attribute is a C++17 extension">, InGroup;
+def ext_cxx2a_attr : Extension<
+  "use of the %0 attribute is a C++2a extension">, InGroup;
 
 def warn_unused_comparison : Warning<
   "%select{equality|inequality|relational|three-way}0 comparison result unused">,
@@ -8158,6 +8160,9 @@
   "fallthrough annotation in unreachable code">,
   InGroup, DefaultIgnore;
 
+def err_likely_attr_invalid_placement : Error<
+  "likely annotation can't appear here">;
+
 def warn_unreachable_default : Warning<
   "default label in switch which covers all enumeration values">,
   InGroup, DefaultIgnore;
Index: clang/include/clang/Basic/AttrDocs.td
===

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

[Gauthier via Phabricator via cfe-commits]

Tyker updated this revision to Diff 191031.
Tyker added a comment.

i added tests as you requested. i didn't add test for the codegen as it wasn't 
affected.


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

https://reviews.llvm.org/D59467

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/AST/ast-dump-attr.cpp

Index: clang/test/AST/ast-dump-attr.cpp
===
--- clang/test/AST/ast-dump-attr.cpp
+++ clang/test/AST/ast-dump-attr.cpp
@@ -23,6 +23,21 @@
 // CHECK-NEXT:   FallThroughAttr
 // CHECK-NEXT:   NullStmt
 
+int TestLikelyAttribute(int i) {
+  if (i == 1) [[likely]] {
+return 0;
+  } else if (i == 2) [[likely]]
+return 1;
+  return 2;
+}
+// CHECK:  FunctionDecl{{.*}}TestLikelyAttribute
+// CHECK:  AttributedStmt
+// CHECK-NEXT: LikelyAttr
+// CHECK-NEXT: IfStmt
+// CHECK:  AttributedStmt
+// CHECK-NEXT: LikelyAttr
+// CHECK-NEXT: IfStmt
+
 [[clang::warn_unused_result]] int TestCXX11DeclAttr();
 // CHECK:  FunctionDecl{{.*}}TestCXX11DeclAttr
 // CHECK-NEXT:   WarnUnusedResultAttr
Index: clang/lib/Sema/SemaStmtAttr.cpp
===
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -51,6 +51,21 @@
   return ::new (S.Context) auto(Attr);
 }
 
+static Attr *handleLikelyAttr(Sema , Stmt *St, const ParsedAttr ,
+   SourceRange Range) {
+  LikelyAttr Attr(A.getRange(), S.Context,
+   A.getAttributeSpellingListIndex());
+
+  if (!llvm::isa(St)) {
+S.Diag(A.getLoc(), diag::err_likely_attr_invalid_placement) << A.getName();
+  }
+
+  if (!S.getLangOpts().CPlusPlus2a && !A.getScopeName())
+S.Diag(A.getLoc(), diag::ext_cxx2a_attr) << A.getName();
+
+  return ::new (S.Context) auto(Attr);
+}
+
 static Attr *handleSuppressAttr(Sema , Stmt *St, const ParsedAttr ,
 SourceRange Range) {
   if (A.getNumArgs() < 1) {
@@ -336,6 +351,8 @@
 return nullptr;
   case ParsedAttr::AT_FallThrough:
 return handleFallThroughAttr(S, St, A, Range);
+  case ParsedAttr::AT_Likely:
+return handleLikelyAttr(S, St, A, Range);
   case ParsedAttr::AT_LoopHint:
 return handleLoopHintAttr(S, St, A, Range);
   case ParsedAttr::AT_OpenCLUnrollHint:
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -1216,6 +1216,9 @@
 : Sema::ConditionKind::Boolean))
 return StmtError();
 
+  ParsedAttributesWithRange Attrs(AttrFactory);
+  MaybeParseCXX11Attributes(Attrs);
+
   llvm::Optional ConstexprCondition;
   if (IsConstexpr)
 ConstexprCondition = Cond.getKnownValue();
@@ -1314,8 +1317,13 @@
   if (ElseStmt.isInvalid())
 ElseStmt = Actions.ActOnNullStmt(ElseStmtLoc);
 
-  return Actions.ActOnIfStmt(IfLoc, IsConstexpr, InitStmt.get(), Cond,
- ThenStmt.get(), ElseLoc, ElseStmt.get());
+  StmtResult Stmt = Actions.ActOnIfStmt(IfLoc, IsConstexpr, InitStmt.get(), Cond,
+ ThenStmt.get(), ElseLoc, ElseStmt.get());
+
+  if (!Attrs.empty())
+return Actions.ProcessStmtAttributes(Stmt.get(), Attrs, Attrs.Range);
+
+  return Stmt;
 }
 
 /// ParseSwitchStatement
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -3858,6 +3858,7 @@
   case ParsedAttr::AT_Deprecated:
   case ParsedAttr::AT_FallThrough:
   case ParsedAttr::AT_CXX11NoReturn:
+  case ParsedAttr::AT_Likely:
 return true;
   case ParsedAttr::AT_WarnUnusedResult:
 return !ScopeName && AttrName->getName().equals("nodiscard");
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7339,6 +7339,8 @@
   "use of the %0 attribute is a C++14 extension">, InGroup;
 def ext_cxx17_attr : Extension<
   "use of the %0 attribute is a C++17 extension">, InGroup;
+def ext_cxx2a_attr : Extension<
+  "use of the %0 attribute is a C++2a extension">, InGroup;
 
 def warn_unused_comparison : Warning<
   "%select{equality|inequality|relational|three-way}0 comparison result unused">,
@@ -8158,6 +8160,9 @@
   "fallthrough annotation in unreachable code">,
   InGroup, DefaultIgnore;
 
+def err_likely_attr_invalid_placement : Error<
+  "likely annotation can't appear here">;
+
 def warn_unreachable_default : Warning<
   "default label in switch which covers all enumeration values">,
   InGroup, DefaultIgnore;

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Please always upload all patches with full context (`-U9`)
Misses tests (sema, ast-dump, codegen)




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8164
+def err_likely_attr_invalid_placement : Error<
+  "likely annotation can't apear here">;
+

appear


Repository:
  rC Clang

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 Likely Attribute from C++2a to AST

[Gauthier via Phabricator via cfe-commits]

Tyker created this revision.
Tyker added a reviewer: aaron.ballman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

attributes after an if like:

if (...) [[likely]]

are now applied on the if instead of the following statement.

i added the likely attribute in the necessary places Attr.td, AttrDocs.td.

i added a diagnostics kind for C++2a and for likely not used on an if statement.


Repository:
  rC Clang

https://reviews.llvm.org/D59467

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp

Index: clang/lib/Sema/SemaStmtAttr.cpp
===
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -51,6 +51,22 @@
   return ::new (S.Context) auto(Attr);
 }
 
+static Attr *handleLikelyAttr(Sema , Stmt *St, const ParsedAttr ,
+   SourceRange Range) {
+  LikelyAttr Attr(A.getRange(), S.Context,
+   A.getAttributeSpellingListIndex());
+
+  if (!llvm::isa(St)) {
+S.Diag(A.getLoc(), diag::err_likely_attr_invalid_placement) << A.getName();
+  }
+
+  if (!S.getLangOpts().CPlusPlus2a && A.isCXX11Attribute() &&
+  !A.getScopeName())
+S.Diag(A.getLoc(), diag::ext_cxx2a_attr) << A.getName();
+
+  return ::new (S.Context) auto(Attr);
+}
+
 static Attr *handleSuppressAttr(Sema , Stmt *St, const ParsedAttr ,
 SourceRange Range) {
   if (A.getNumArgs() < 1) {
@@ -336,6 +352,8 @@
 return nullptr;
   case ParsedAttr::AT_FallThrough:
 return handleFallThroughAttr(S, St, A, Range);
+  case ParsedAttr::AT_Likely:
+return handleLikelyAttr(S, St, A, Range);
   case ParsedAttr::AT_LoopHint:
 return handleLoopHintAttr(S, St, A, Range);
   case ParsedAttr::AT_OpenCLUnrollHint:
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -1216,6 +1216,9 @@
 : Sema::ConditionKind::Boolean))
 return StmtError();
 
+  ParsedAttributesWithRange Attrs(AttrFactory);
+  MaybeParseCXX11Attributes(Attrs);
+
   llvm::Optional ConstexprCondition;
   if (IsConstexpr)
 ConstexprCondition = Cond.getKnownValue();
@@ -1314,8 +1317,12 @@
   if (ElseStmt.isInvalid())
 ElseStmt = Actions.ActOnNullStmt(ElseStmtLoc);
 
-  return Actions.ActOnIfStmt(IfLoc, IsConstexpr, InitStmt.get(), Cond,
- ThenStmt.get(), ElseLoc, ElseStmt.get());
+  StmtResult Stmt = Actions.ActOnIfStmt(IfLoc, IsConstexpr, InitStmt.get(), Cond,
+ ThenStmt.get(), ElseLoc, ElseStmt.get());
+
+  if (!Attrs.empty())
+return Actions.ProcessStmtAttributes(Stmt.get(), Attrs, Attrs.Range);
+  return Stmt;
 }
 
 /// ParseSwitchStatement
Index: clang/lib/Parse/ParseDeclCXX.cpp
===
--- clang/lib/Parse/ParseDeclCXX.cpp
+++ clang/lib/Parse/ParseDeclCXX.cpp
@@ -3858,6 +3858,7 @@
   case ParsedAttr::AT_Deprecated:
   case ParsedAttr::AT_FallThrough:
   case ParsedAttr::AT_CXX11NoReturn:
+  case ParsedAttr::AT_Likely:
 return true;
   case ParsedAttr::AT_WarnUnusedResult:
 return !ScopeName && AttrName->getName().equals("nodiscard");
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7339,6 +7339,8 @@
   "use of the %0 attribute is a C++14 extension">, InGroup;
 def ext_cxx17_attr : Extension<
   "use of the %0 attribute is a C++17 extension">, InGroup;
+def ext_cxx2a_attr : Extension<
+  "use of the %0 attribute is a C++2a extension">, InGroup;
 
 def warn_unused_comparison : Warning<
   "%select{equality|inequality|relational|three-way}0 comparison result unused">,
@@ -8158,6 +8160,9 @@
   "fallthrough annotation in unreachable code">,
   InGroup, DefaultIgnore;
 
+def err_likely_attr_invalid_placement : Error<
+  "likely annotation can't apear here">;
+
 def warn_unreachable_default : Warning<
   "default label in switch which covers all enumeration values">,
   InGroup, DefaultIgnore;
Index: clang/include/clang/Basic/AttrDocs.td
===
--- clang/include/clang/Basic/AttrDocs.td
+++ clang/include/clang/Basic/AttrDocs.td
@@ -1492,6 +1492,26 @@
   }];
 }
 
+def LikelyDocs : Documentation {
+  let Category = DocCatStmt;
+  let Heading = "likely";
+  let Content = [{
+The ``likely`` (or ``clang::likely``) attribute is used to annotate that a condition is likely to be true
+this is supposed to be used as a hint by the optimizer (not yet implemented)
+
+Here is an example:
+
+..