[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Mark de Wever via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.
Closed by commit rG08196e0b2e1f: Implements [[likely]] and [[unlikely]] in 
IfStmt. (authored by Mordante).

Changed prior to commit:
  https://reviews.llvm.org/D85091?vs=289979=290787#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85091

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/lib/AST/Stmt.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGenCXX/attr-likelihood-if-branch-weights.cpp
  clang/test/Preprocessor/has_attribute.cpp
  clang/test/Sema/attr-likelihood.c
  clang/test/SemaCXX/attr-likelihood.cpp
  clang/www/cxx_status.html
  llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp

Index: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
===
--- llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
+++ llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
@@ -24,7 +24,6 @@
 #include "llvm/IR/Metadata.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
-#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Utils/MisExpect.h"
@@ -48,10 +47,10 @@
 // 'select' instructions. It may be worthwhile to hoist these values to some
 // shared space, so they can be used directly by other passes.
 
-static cl::opt LikelyBranchWeight(
+cl::opt llvm::LikelyBranchWeight(
 "likely-branch-weight", cl::Hidden, cl::init(2000),
 cl::desc("Weight of the branch likely to be taken (default = 2000)"));
-static cl::opt UnlikelyBranchWeight(
+cl::opt llvm::UnlikelyBranchWeight(
 "unlikely-branch-weight", cl::Hidden, cl::init(1),
 cl::desc("Weight of the branch unlikely to be taken (default = 1)"));
 
Index: llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
===
--- llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
+++ llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
@@ -17,6 +17,7 @@
 
 #include "llvm/IR/Function.h"
 #include "llvm/IR/PassManager.h"
+#include "llvm/Support/CommandLine.h"
 
 namespace llvm {
 
@@ -31,6 +32,8 @@
   PreservedAnalyses run(Function , FunctionAnalysisManager &);
 };
 
+extern cl::opt LikelyBranchWeight;
+extern cl::opt UnlikelyBranchWeight;
 }
 
 #endif
Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -987,7 +987,7 @@
 
   [[likely]] and [[unlikely]] attributes
   https://wg21.link/p0479r5;>P0479R5
-  No
+  Clang 12 (partial)
 
 
   typename optional in more contexts
Index: clang/test/SemaCXX/attr-likelihood.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-likelihood.cpp
@@ -0,0 +1,132 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify
+// RUN: %clang_cc1 %s -DPEDANTIC -pedantic -fsyntax-only -verify
+
+#if PEDANTIC
+void g() {
+  if (true)
+[[likely]] {} // expected-warning {{use of the 'likely' attribute is a C++20 extension}}
+  else
+[[unlikely]] {} // expected-warning {{use of the 'unlikely' attribute is a C++20 extension}}
+}
+#else
+void a() {
+  if (true)
+[[likely]]; // expected-warning {{conflicting attributes 'likely' are ignored}}
+  else
+[[likely]]; // expected-note {{conflicting attribute is here}}
+}
+
+void b() {
+  if (true)
+[[unlikely]]; // expected-warning {{conflicting attributes 'unlikely' are ignored}}
+  else
+[[unlikely]]; // expected-note {{conflicting attribute is here}}
+}
+
+void c() {
+  if (true)
+[[likely]];
+}
+
+void d() {
+  if (true)
+[[unlikely]];
+}
+
+void g() {
+  if (true)
+[[likely]] {}
+  else
+[[unlikely]] {}
+}
+
+void h() {
+  if (true)
+[[likely]] {}
+  else {
+  }
+}
+
+void i() {
+  if (true)
+[[unlikely]] {}
+  else {
+  }
+}
+
+void j() {
+  if (true) {
+  } else
+[[likely]] {}
+}
+
+void k() {
+  if (true) {
+  } else
+[[likely]] {}
+}
+
+void l() {
+  if (true)
+[[likely]] {}
+  else
+[[unlikely]] if (false) [[likely]] {}
+}
+
+void m() {
+  [[likely]] int x = 42; // expected-error {{'likely' attribute cannot be applied to a declaration}}
+
+  if (x)
+[[unlikely]] {}
+  if (x) {
+[[unlikely]];
+  }
+  switch (x) {
+  case 1:
+[[likely]] {}
+break;
+[[likely]] case 2 : case 3 : {}
+break;
+  }
+
+  do {
+[[unlikely]];
+  } while 

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 2 inline comments as done.
Mordante added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:1180
+
+  /// \returns the likelihood of the branches of an if statement.
+  static Likelihood getLikelihood(const Stmt *Then, const Stmt *Else);

Quuxplusone wrote:
> Mordante wrote:
> > aaron.ballman wrote:
> > > This only returns one value, so it can't return the likelihood of "the 
> > > branches". How about something along the lines of:
> > > 
> > > `returns the likelihood of the 'then' branch of an 'if' statement. The 
> > > 'else' branch is required to determine whether both branches specify the 
> > > same likelihood, which impacts the result.`
> > Good catch, I'll use your suggestion.
> s/impacts/affects/ ;)
Just in time before I commit this patch :-)


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

https://reviews.llvm.org/D85091

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:1180
+
+  /// \returns the likelihood of the branches of an if statement.
+  static Likelihood getLikelihood(const Stmt *Then, const Stmt *Else);

Mordante wrote:
> aaron.ballman wrote:
> > This only returns one value, so it can't return the likelihood of "the 
> > branches". How about something along the lines of:
> > 
> > `returns the likelihood of the 'then' branch of an 'if' statement. The 
> > 'else' branch is required to determine whether both branches specify the 
> > same likelihood, which impacts the result.`
> Good catch, I'll use your suggestion.
s/impacts/affects/ ;)


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

https://reviews.llvm.org/D85091

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Mark de Wever via Phabricator via cfe-commits]

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

Thanks for your feedback during the review.




Comment at: clang/include/clang/AST/Stmt.h:1180
+
+  /// \returns the likelihood of the branches of an if statement.
+  static Likelihood getLikelihood(const Stmt *Then, const Stmt *Else);

aaron.ballman wrote:
> This only returns one value, so it can't return the likelihood of "the 
> branches". How about something along the lines of:
> 
> `returns the likelihood of the 'then' branch of an 'if' statement. The 'else' 
> branch is required to determine whether both branches specify the same 
> likelihood, which impacts the result.`
Good catch, I'll use your suggestion.


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

https://reviews.llvm.org/D85091

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from a commenting nit. Thank you for the work on this!




Comment at: clang/include/clang/AST/Stmt.h:1180
+
+  /// \returns the likelihood of the branches of an if statement.
+  static Likelihood getLikelihood(const Stmt *Then, const Stmt *Else);

This only returns one value, so it can't return the likelihood of "the 
branches". How about something along the lines of:

`returns the likelihood of the 'then' branch of an 'if' statement. The 'else' 
branch is required to determine whether both branches specify the same 
likelihood, which impacts the result.`


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

https://reviews.llvm.org/D85091

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Mark de Wever via Phabricator via cfe-commits]

Mordante updated this revision to Diff 289979.
Mordante marked an inline comment as done.
Mordante added a comment.

Addresses review comments, mainly:

- Improving the documentation
- Removing the AST bits since they're no longer required


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

https://reviews.llvm.org/D85091

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/lib/AST/Stmt.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGenCXX/attr-likelihood-if-branch-weights.cpp
  clang/test/Preprocessor/has_attribute.cpp
  clang/test/Sema/attr-likelihood.c
  clang/test/SemaCXX/attr-likelihood.cpp
  clang/www/cxx_status.html
  llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp

Index: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
===
--- llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
+++ llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
@@ -24,7 +24,6 @@
 #include "llvm/IR/Metadata.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
-#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Utils/MisExpect.h"
@@ -48,10 +47,10 @@
 // 'select' instructions. It may be worthwhile to hoist these values to some
 // shared space, so they can be used directly by other passes.
 
-static cl::opt LikelyBranchWeight(
+cl::opt llvm::LikelyBranchWeight(
 "likely-branch-weight", cl::Hidden, cl::init(2000),
 cl::desc("Weight of the branch likely to be taken (default = 2000)"));
-static cl::opt UnlikelyBranchWeight(
+cl::opt llvm::UnlikelyBranchWeight(
 "unlikely-branch-weight", cl::Hidden, cl::init(1),
 cl::desc("Weight of the branch unlikely to be taken (default = 1)"));
 
Index: llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
===
--- llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
+++ llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
@@ -17,6 +17,7 @@
 
 #include "llvm/IR/Function.h"
 #include "llvm/IR/PassManager.h"
+#include "llvm/Support/CommandLine.h"
 
 namespace llvm {
 
@@ -31,6 +32,8 @@
   PreservedAnalyses run(Function , FunctionAnalysisManager &);
 };
 
+extern cl::opt LikelyBranchWeight;
+extern cl::opt UnlikelyBranchWeight;
 }
 
 #endif
Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -987,7 +987,7 @@
 
   [[likely]] and [[unlikely]] attributes
   https://wg21.link/p0479r5;>P0479R5
-  No
+  Clang 12 (partial)
 
 
   typename optional in more contexts
Index: clang/test/SemaCXX/attr-likelihood.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-likelihood.cpp
@@ -0,0 +1,132 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify
+// RUN: %clang_cc1 %s -DPEDANTIC -pedantic -fsyntax-only -verify
+
+#if PEDANTIC
+void g() {
+  if (true)
+[[likely]] {} // expected-warning {{use of the 'likely' attribute is a C++20 extension}}
+  else
+[[unlikely]] {} // expected-warning {{use of the 'unlikely' attribute is a C++20 extension}}
+}
+#else
+void a() {
+  if (true)
+[[likely]]; // expected-warning {{conflicting attributes 'likely' are ignored}}
+  else
+[[likely]]; // expected-note {{conflicting attribute is here}}
+}
+
+void b() {
+  if (true)
+[[unlikely]]; // expected-warning {{conflicting attributes 'unlikely' are ignored}}
+  else
+[[unlikely]]; // expected-note {{conflicting attribute is here}}
+}
+
+void c() {
+  if (true)
+[[likely]];
+}
+
+void d() {
+  if (true)
+[[unlikely]];
+}
+
+void g() {
+  if (true)
+[[likely]] {}
+  else
+[[unlikely]] {}
+}
+
+void h() {
+  if (true)
+[[likely]] {}
+  else {
+  }
+}
+
+void i() {
+  if (true)
+[[unlikely]] {}
+  else {
+  }
+}
+
+void j() {
+  if (true) {
+  } else
+[[likely]] {}
+}
+
+void k() {
+  if (true) {
+  } else
+[[likely]] {}
+}
+
+void l() {
+  if (true)
+[[likely]] {}
+  else
+[[unlikely]] if (false) [[likely]] {}
+}
+
+void m() {
+  [[likely]] int x = 42; // expected-error {{'likely' attribute cannot be applied to a declaration}}
+
+  if (x)
+[[unlikely]] {}
+  if (x) {
+[[unlikely]];
+  }
+  switch (x) {
+  case 1:
+[[likely]] {}
+break;
+[[likely]] case 2 : case 3 : {}
+break;
+  }
+
+  do {
+[[unlikely]];
+  } while (x);
+  do
+[[unlikely]] {}
+  while (x);
+  do { // expected-note {{to match this 'do'}}
+  }
+  

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked an inline comment as done.
Mordante added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:578
+static std::pair
+getLikelihood(const Stmt *Stmt) {
+  if (const auto *AS = dyn_cast(Stmt))

aaron.ballman wrote:
> Mordante wrote:
> > rsmith wrote:
> > > Mordante wrote:
> > > > aaron.ballman wrote:
> > > > > rsmith wrote:
> > > > > > Sema doesn't care about any of this; can you move this code to 
> > > > > > CodeGen and remove the code that stores likelihood data in the AST?
> > > > > FWIW, I asked for it to be moved out of CodeGen and into Sema because 
> > > > > the original implementation in CodeGen was doing a fair amount of 
> > > > > work trying to interrogate the AST for this information. Now that 
> > > > > we've switched the design to only be on the substatement of an 
> > > > > if/else statement (rather than an arbitrary statement), it may be 
> > > > > that CodeGen is a better place for this again (and if so, I'm sorry 
> > > > > for the churn).
> > > > At the moment the Sema cares about it to generate diagnostics about 
> > > > conflicting annotations:
> > > > ```
> > > > if(i) [[ likely]] {}
> > > > else [[likely]] {}
> > > > ```
> > > > This diagnostic only happens for an if statement, for a switch multiple 
> > > > values can be considered likely.
> > > > Do you prefer to have the diagnostic also in the CodeGen?
> > > It looked to me like you'd reached agreement to remove that diagnostic. 
> > > Are you intending to keep it?
> > > 
> > > If so, then I'd suggest you make `getLikelihood` a member of `Stmt` so 
> > > that `CodeGen` and the warning here can both easily call it.
> > @aaron.ballman I thought we wanted to keep this conflict warning, am I 
> > correct?
> > I'll add an extra function to the Stmt to test for a conflict and use that 
> > for the diagnostic in the Sema. This allows me to use `LH_None` for no 
> > attribute or a conflict of attributes in the CodeGen. Then there's no need 
> > for `LH_Conflict` and it can be removed from the enumerate.
> > @aaron.ballman I thought we wanted to keep this conflict warning, am I 
> > correct?
> 
> I want to keep the property that any time the user writes an explicit 
> annotation in their code but we decide to ignore the annotation (for whatever 
> reason), the user gets some sort of diagnostic telling them their 
> expectations may not be met. Because we're ignoring the attributes in the 
> case where they're identical on both branches, I'd like to keep some form of 
> diagnostic.
Good then we're on the same page.


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

https://reviews.llvm.org/D85091

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D85091#2252632 , @Mordante wrote:

> In D85091#2250657 , @rsmith wrote:
>
>> Looking specifically for attributes in the 'then' and 'else' cases of an 
>> `if` seems like a fine first pass at this, but I think this is the wrong way 
>> to lower these attributes in the longer term: we should have a uniform 
>> treatment of them that looks for the most recent prior branch and annotates 
>> it with weights. We could do that either by generating LLVM intrinsic calls 
>> that an LLVM pass lowers, or by having the frontend look backwards for the 
>> most recent branch and annotate it. I suppose it's instructive to consider a 
>> case such as:
>>
>>   void mainloop() noexcept; // probably terminates by calling `exit`
>>   void f() {
>> mainloop();
>> [[unlikely]];
>> somethingelse();
>>   }
>>
>> ... where the `[[unlikely]];` should probably cause us to split the 
>> `somethingelse()` out into a separate basic block that we mark as cold, but 
>> should not cause `f()` itself or its call to `mainloop()` to be considered 
>> unlikely -- except in the case where `mainloop()` can be proven to always 
>> terminate, in which case the implication is that it's unlikely that `f()` is 
>> invoked. Cases like this probably need the LLVM intrinsic approach to model 
>> them properly.
>
> We indeed considered similar cases and wondered whether it was really 
> intended to work this way. Since this approach seems a bit hard to explain to 
> users, I changed the code back to only allow the attribute on the 
> substatement of the `if` and `else`. For now I first want to implement the 
> simple approach, which I expect will be the most common use case. Once 
> finished we can see what the next steps will be.

+1 to the cautious approach. While I can understand how to implement what 
Richard is suggesting, I'm not convinced it results in readable code or that 
we're missing important optimization cases by using the more restrictive 
approach, so I'd like to get some field experience with users first.


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

https://reviews.llvm.org/D85091

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:578
+static std::pair
+getLikelihood(const Stmt *Stmt) {
+  if (const auto *AS = dyn_cast(Stmt))

Mordante wrote:
> rsmith wrote:
> > Mordante wrote:
> > > aaron.ballman wrote:
> > > > rsmith wrote:
> > > > > Sema doesn't care about any of this; can you move this code to 
> > > > > CodeGen and remove the code that stores likelihood data in the AST?
> > > > FWIW, I asked for it to be moved out of CodeGen and into Sema because 
> > > > the original implementation in CodeGen was doing a fair amount of work 
> > > > trying to interrogate the AST for this information. Now that we've 
> > > > switched the design to only be on the substatement of an if/else 
> > > > statement (rather than an arbitrary statement), it may be that CodeGen 
> > > > is a better place for this again (and if so, I'm sorry for the churn).
> > > At the moment the Sema cares about it to generate diagnostics about 
> > > conflicting annotations:
> > > ```
> > > if(i) [[ likely]] {}
> > > else [[likely]] {}
> > > ```
> > > This diagnostic only happens for an if statement, for a switch multiple 
> > > values can be considered likely.
> > > Do you prefer to have the diagnostic also in the CodeGen?
> > It looked to me like you'd reached agreement to remove that diagnostic. Are 
> > you intending to keep it?
> > 
> > If so, then I'd suggest you make `getLikelihood` a member of `Stmt` so that 
> > `CodeGen` and the warning here can both easily call it.
> @aaron.ballman I thought we wanted to keep this conflict warning, am I 
> correct?
> I'll add an extra function to the Stmt to test for a conflict and use that 
> for the diagnostic in the Sema. This allows me to use `LH_None` for no 
> attribute or a conflict of attributes in the CodeGen. Then there's no need 
> for `LH_Conflict` and it can be removed from the enumerate.
> @aaron.ballman I thought we wanted to keep this conflict warning, am I 
> correct?

I want to keep the property that any time the user writes an explicit 
annotation in their code but we decide to ignore the annotation (for whatever 
reason), the user gets some sort of diagnostic telling them their expectations 
may not be met. Because we're ignoring the attributes in the case where they're 
identical on both branches, I'd like to keep some form of diagnostic.


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

https://reviews.llvm.org/D85091

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 2 inline comments as done.
Mordante added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:578
+static std::pair
+getLikelihood(const Stmt *Stmt) {
+  if (const auto *AS = dyn_cast(Stmt))

rsmith wrote:
> Mordante wrote:
> > aaron.ballman wrote:
> > > rsmith wrote:
> > > > Sema doesn't care about any of this; can you move this code to CodeGen 
> > > > and remove the code that stores likelihood data in the AST?
> > > FWIW, I asked for it to be moved out of CodeGen and into Sema because the 
> > > original implementation in CodeGen was doing a fair amount of work trying 
> > > to interrogate the AST for this information. Now that we've switched the 
> > > design to only be on the substatement of an if/else statement (rather 
> > > than an arbitrary statement), it may be that CodeGen is a better place 
> > > for this again (and if so, I'm sorry for the churn).
> > At the moment the Sema cares about it to generate diagnostics about 
> > conflicting annotations:
> > ```
> > if(i) [[ likely]] {}
> > else [[likely]] {}
> > ```
> > This diagnostic only happens for an if statement, for a switch multiple 
> > values can be considered likely.
> > Do you prefer to have the diagnostic also in the CodeGen?
> It looked to me like you'd reached agreement to remove that diagnostic. Are 
> you intending to keep it?
> 
> If so, then I'd suggest you make `getLikelihood` a member of `Stmt` so that 
> `CodeGen` and the warning here can both easily call it.
@aaron.ballman I thought we wanted to keep this conflict warning, am I correct?
I'll add an extra function to the Stmt to test for a conflict and use that for 
the diagnostic in the Sema. This allows me to use `LH_None` for no attribute or 
a conflict of attributes in the CodeGen. Then there's no need for `LH_Conflict` 
and it can be removed from the enumerate.


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

https://reviews.llvm.org/D85091

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/AST/Stmt.h:1105-
+  /// The likelihood of a branch being taken.
+  enum Likelihood {
+LH_None, ///< No attribute set.
+LH_Likely,   ///< Branch has the [[likely]] attribute.
+LH_Unlikely, ///< Branch has the [[unlikely]] attribute.
+LH_Conflict  ///< Both branches of the IfStmt have the same attribute.
+  };

I'd suggest you order these in increasing order of likeliness: unlikely, none, 
likely. Then you can determine what branch weights to use by a three-way 
comparison of the likeliness of the `then` branch and the likeliness of the 
`else` branch.



Comment at: clang/lib/Sema/SemaStmt.cpp:578
+static std::pair
+getLikelihood(const Stmt *Stmt) {
+  if (const auto *AS = dyn_cast(Stmt))

Mordante wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > Sema doesn't care about any of this; can you move this code to CodeGen 
> > > and remove the code that stores likelihood data in the AST?
> > FWIW, I asked for it to be moved out of CodeGen and into Sema because the 
> > original implementation in CodeGen was doing a fair amount of work trying 
> > to interrogate the AST for this information. Now that we've switched the 
> > design to only be on the substatement of an if/else statement (rather than 
> > an arbitrary statement), it may be that CodeGen is a better place for this 
> > again (and if so, I'm sorry for the churn).
> At the moment the Sema cares about it to generate diagnostics about 
> conflicting annotations:
> ```
> if(i) [[ likely]] {}
> else [[likely]] {}
> ```
> This diagnostic only happens for an if statement, for a switch multiple 
> values can be considered likely.
> Do you prefer to have the diagnostic also in the CodeGen?
It looked to me like you'd reached agreement to remove that diagnostic. Are you 
intending to keep it?

If so, then I'd suggest you make `getLikelihood` a member of `Stmt` so that 
`CodeGen` and the warning here can both easily call it.


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

https://reviews.llvm.org/D85091

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 8 inline comments as done.
Mordante added a comment.

In D85091#2250657 , @rsmith wrote:

> Looking specifically for attributes in the 'then' and 'else' cases of an `if` 
> seems like a fine first pass at this, but I think this is the wrong way to 
> lower these attributes in the longer term: we should have a uniform treatment 
> of them that looks for the most recent prior branch and annotates it with 
> weights. We could do that either by generating LLVM intrinsic calls that an 
> LLVM pass lowers, or by having the frontend look backwards for the most 
> recent branch and annotate it. I suppose it's instructive to consider a case 
> such as:
>
>   void mainloop() noexcept; // probably terminates by calling `exit`
>   void f() {
> mainloop();
> [[unlikely]];
> somethingelse();
>   }
>
> ... where the `[[unlikely]];` should probably cause us to split the 
> `somethingelse()` out into a separate basic block that we mark as cold, but 
> should not cause `f()` itself or its call to `mainloop()` to be considered 
> unlikely -- except in the case where `mainloop()` can be proven to always 
> terminate, in which case the implication is that it's unlikely that `f()` is 
> invoked. Cases like this probably need the LLVM intrinsic approach to model 
> them properly.

We indeed considered similar cases and wondered whether it was really intended 
to work this way. Since this approach seems a bit hard to explain to users, I 
changed the code back to only allow the attribute on the substatement of the 
`if` and `else`. For now I first want to implement the simple approach, which I 
expect will be the most common use case. Once finished we can see what the next 
steps will be.




Comment at: clang/include/clang/AST/Stmt.h:175-178
+/// The likelihood of taking the ThenStmt.
+/// One of the enumeration values in Stmt::Likelihood.
+unsigned ThenLikelihood : 2;
+

rsmith wrote:
> Do we need to store this here? The "then" and "else" cases should be an 
> `AttributedStmt` holding the likelihood attribute, so duplicating that info 
> here seems redundant.
Agreed. I'll remove it again.



Comment at: clang/lib/Sema/SemaStmt.cpp:578
+static std::pair
+getLikelihood(const Stmt *Stmt) {
+  if (const auto *AS = dyn_cast(Stmt))

aaron.ballman wrote:
> rsmith wrote:
> > Sema doesn't care about any of this; can you move this code to CodeGen and 
> > remove the code that stores likelihood data in the AST?
> FWIW, I asked for it to be moved out of CodeGen and into Sema because the 
> original implementation in CodeGen was doing a fair amount of work trying to 
> interrogate the AST for this information. Now that we've switched the design 
> to only be on the substatement of an if/else statement (rather than an 
> arbitrary statement), it may be that CodeGen is a better place for this again 
> (and if so, I'm sorry for the churn).
At the moment the Sema cares about it to generate diagnostics about conflicting 
annotations:
```
if(i) [[ likely]] {}
else [[likely]] {}
```
This diagnostic only happens for an if statement, for a switch multiple values 
can be considered likely.
Do you prefer to have the diagnostic also in the CodeGen?


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

https://reviews.llvm.org/D85091

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:578
+static std::pair
+getLikelihood(const Stmt *Stmt) {
+  if (const auto *AS = dyn_cast(Stmt))

rsmith wrote:
> Sema doesn't care about any of this; can you move this code to CodeGen and 
> remove the code that stores likelihood data in the AST?
FWIW, I asked for it to be moved out of CodeGen and into Sema because the 
original implementation in CodeGen was doing a fair amount of work trying to 
interrogate the AST for this information. Now that we've switched the design to 
only be on the substatement of an if/else statement (rather than an arbitrary 
statement), it may be that CodeGen is a better place for this again (and if so, 
I'm sorry for the churn).


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

https://reviews.llvm.org/D85091

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Looking specifically for attributes in the 'then' and 'else' cases of an `if` 
seems like a fine first pass at this, but I think this is the wrong way to 
lower these attributes in the longer term: we should have a uniform treatment 
of them that looks for the most recent prior branch and annotates it with 
weights. We could do that either by generating LLVM intrinsic calls that an 
LLVM pass lowers, or by having the frontend look backwards for the most recent 
branch and annotate it. I suppose it's instructive to consider a case such as:

  void mainloop() noexcept; // probably terminates by calling `exit`
  void f() {
mainloop();
[[unlikely]];
somethingelse();
  }

... where the `[[unlikely]];` should probably cause us to split the 
`somethingelse()` out into a separate basic block that we mark as cold, but 
should not cause `f()` itself or its call to `mainloop()` to be considered 
unlikely -- except in the case where `mainloop()` can be proven to always 
terminate, in which case the implication is that it's unlikely that `f()` is 
invoked. Cases like this probably need the LLVM intrinsic approach to model 
them properly.




Comment at: clang/include/clang/AST/Stmt.h:175-178
+/// The likelihood of taking the ThenStmt.
+/// One of the enumeration values in Stmt::Likelihood.
+unsigned ThenLikelihood : 2;
+

Do we need to store this here? The "then" and "else" cases should be an 
`AttributedStmt` holding the likelihood attribute, so duplicating that info 
here seems redundant.



Comment at: clang/lib/Sema/SemaStmt.cpp:578
+static std::pair
+getLikelihood(const Stmt *Stmt) {
+  if (const auto *AS = dyn_cast(Stmt))

Sema doesn't care about any of this; can you move this code to CodeGen and 
remove the code that stores likelihood data in the AST?


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

https://reviews.llvm.org/D85091

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 8 inline comments as done.
Mordante added a comment.

Thanks again for your feedback. Once we agree on what to do with the Likelihood 
state in the output I'll finish the changes and submit a new patch.




Comment at: clang/include/clang/Basic/AttrDocs.td:1697
+It isn't allowed to annotate a single statement with both ``likely`` and
+``unlikely``. It's allowed to create a contradictions by marking
+the first statement in the ``true`` and ``false`` branch with the same

aaron.ballman wrote:
> This sentence and the next one seem to say opposite things. I think this 
> should read:
> 
> `Annotating the 'true' and 'false' branch of an 'if' statement with the same 
> likelihood attribute will result in a diagnostic and the attributes are 
> ignored on both branches.`
I think the current wording is correct, but I like your wording better. So I'll 
use your wording.



Comment at: clang/include/clang/Basic/AttrDocs.td:1705
+
+In Clang the attributes will be ignored if they're not placed on the first
+statement in a branch. The C++ Standard recommends to honor them on every

aaron.ballman wrote:
> How about: `In Clang, the attributes will be ignored if they're not placed on 
> the substatement of a selection statement.`
Your wording is correct for an `if` statement, but it will be different in a 
`switch` statement. I'll use your wording but replace `selection statement` 
with `if or else statement`. I like the substatement instead of first statement.



Comment at: clang/include/clang/Basic/AttrDocs.td:1726
+foo(b);
+   // Whether or not the next statement is in the path of execution depends
+// on the declaration of foo():

aaron.ballman wrote:
> The formatting on this comment looks off (maybe different indentation, or 
> tabs vs spaces).
Good catch, I thought I disabled using tabs.



Comment at: clang/lib/AST/JSONNodeDumper.cpp:1452
   attributeOnlyIfTrue("isConstexpr", IS->isConstexpr());
+  dumpLikelihood(JOS, "thenLikelihood", IS->getThenLikelihood());
 }

aaron.ballman wrote:
> I don't think this change should be necessary because the attribute should be 
> written out for the AST node already. It's worth verifying though.
They are in the AST. I added them to make it clear what the status is. I marked 
a case with a conflict, due to the same attribute in both branches. When the 
attribute is there but not on the substatement it's ignored. I'm not sure 
whether this is too important in the JSON output. I don't know who consumes it. 
So I don't feel to strongly about removing this from the JSON output.

What's your opinion?



Comment at: clang/lib/AST/TextNodeDumper.cpp:927
 OS << " has_else";
+  dumpLikelihood(OS, Node->getThenLikelihood());
 }

aaron.ballman wrote:
> Similar here.
I'm against removing it from the text output. This is be best way to determine 
how the attributes are parsed and processed. Especially when an attribute is 
ignored the likelihood may be different form what's expected. The 
`Node->hasInitStorage()`'s status is also visible in the AST and written out, 
so there are more examples of "duplicated" information.

Do you agree to keep it here?



Comment at: clang/test/AST/ast-dump-if-json.cpp:1333
+// CHECK-NEXT:  "hasElse": true,
+// CHECK-NEXT:  "thenLikelihood": "conflict",
+// CHECK-NEXT:  "inner": [

Here is the likelihood status in the `If` statement.



Comment at: clang/test/AST/ast-dump-if-json.cpp:1429
+// CHECK-NEXT:  "id": "0x{{.*}}",
+// CHECK-NEXT:  "kind": "UnlikelyAttr",
+// CHECK-NEXT:  "range": {

The attribute in the `ThenStmt`.



Comment at: clang/test/AST/ast-dump-if-json.cpp:1483
+// CHECK-NEXT:  "id": "0x{{.*}}",
+// CHECK-NEXT:  "kind": "UnlikelyAttr",
+// CHECK-NEXT:  "range": {

The attribute in the `ElseStmt`.


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

https://reviews.llvm.org/D85091

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1693
+The attributes are used to aid the compiler to determine which branch is
+likely or unlikely to be taken. This is done by marking the first statement in
+the branch with one of the two attributes.

How about: `This is done by marking the branch substatement with one of the two 
attributes.`



Comment at: clang/include/clang/Basic/AttrDocs.td:1697
+It isn't allowed to annotate a single statement with both ``likely`` and
+``unlikely``. It's allowed to create a contradictions by marking
+the first statement in the ``true`` and ``false`` branch with the same

This sentence and the next one seem to say opposite things. I think this should 
read:

`Annotating the 'true' and 'false' branch of an 'if' statement with the same 
likelihood attribute will result in a diagnostic and the attributes are ignored 
on both branches.`



Comment at: clang/include/clang/Basic/AttrDocs.td:1705
+
+In Clang the attributes will be ignored if they're not placed on the first
+statement in a branch. The C++ Standard recommends to honor them on every

How about: `In Clang, the attributes will be ignored if they're not placed on 
the substatement of a selection statement.`



Comment at: clang/include/clang/Basic/AttrDocs.td:1726
+foo(b);
+   // Whether or not the next statement is in the path of execution depends
+// on the declaration of foo():

The formatting on this comment looks off (maybe different indentation, or tabs 
vs spaces).



Comment at: clang/include/clang/Basic/AttrDocs.td:1736
+
+At the moment the attribute only has effect when used in an ``if`` statement.
+

`if` or `else` statement.



Comment at: clang/lib/AST/JSONNodeDumper.cpp:1452
   attributeOnlyIfTrue("isConstexpr", IS->isConstexpr());
+  dumpLikelihood(JOS, "thenLikelihood", IS->getThenLikelihood());
 }

I don't think this change should be necessary because the attribute should be 
written out for the AST node already. It's worth verifying though.



Comment at: clang/lib/AST/TextNodeDumper.cpp:927
 OS << " has_else";
+  dumpLikelihood(OS, Node->getThenLikelihood());
 }

Similar here.



Comment at: clang/lib/Sema/SemaStmt.cpp:618
+
+  // The function returs the likelihood of Then so invert the likelihood of
+  // Else.

returs -> returns


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

https://reviews.llvm.org/D85091

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Mark de Wever via Phabricator via cfe-commits]

Mordante updated this revision to Diff 288781.
Mordante marked an inline comment as done.
Mordante added a comment.

Reworked the patch to match the behaviour as discussed in D86559 
.

- The likelihood attributes only have an effect when used directly on the 
ThenStmt and ElseStmt.
- Conflicting attributes on the ThenStmt and ElseStmt generate a diagnostic.
- Moved the likelihood determination from the CodeGen to the Sema. This 
requires the state to be stored in the AST.
- Updated the AST functions for the new likelihood bits.
- Updated the documentation.

Note currently there's no diagnostic for ignored attributes, this will be added 
in a future patch.


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

https://reviews.llvm.org/D85091

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/Sema.h
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/Stmt.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Parse/ParseDeclCXX.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-if-json.cpp
  clang/test/AST/ast-dump-stmt.cpp
  clang/test/CodeGenCXX/attr-likelihood-if-branch-weights.cpp
  clang/test/Preprocessor/has_attribute.cpp
  clang/test/Sema/attr-likelihood.c
  clang/test/SemaCXX/attr-likelihood.cpp
  clang/www/cxx_status.html
  llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp

Index: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
===
--- llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
+++ llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
@@ -24,7 +24,6 @@
 #include "llvm/IR/Metadata.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
-#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Utils/MisExpect.h"
@@ -48,10 +47,10 @@
 // 'select' instructions. It may be worthwhile to hoist these values to some
 // shared space, so they can be used directly by other passes.
 
-static cl::opt LikelyBranchWeight(
+cl::opt llvm::LikelyBranchWeight(
 "likely-branch-weight", cl::Hidden, cl::init(2000),
 cl::desc("Weight of the branch likely to be taken (default = 2000)"));
-static cl::opt UnlikelyBranchWeight(
+cl::opt llvm::UnlikelyBranchWeight(
 "unlikely-branch-weight", cl::Hidden, cl::init(1),
 cl::desc("Weight of the branch unlikely to be taken (default = 1)"));
 
Index: llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
===
--- llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
+++ llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
@@ -17,6 +17,7 @@
 
 #include "llvm/IR/Function.h"
 #include "llvm/IR/PassManager.h"
+#include "llvm/Support/CommandLine.h"
 
 namespace llvm {
 
@@ -31,6 +32,8 @@
   PreservedAnalyses run(Function , FunctionAnalysisManager &);
 };
 
+extern cl::opt LikelyBranchWeight;
+extern cl::opt UnlikelyBranchWeight;
 }
 
 #endif
Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -987,7 +987,7 @@
 
   [[likely]] and [[unlikely]] attributes
   https://wg21.link/p0479r5;>P0479R5
-  No
+  Clang 12 (partial)
 
 
   typename optional in more contexts
Index: clang/test/SemaCXX/attr-likelihood.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-likelihood.cpp
@@ -0,0 +1,132 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify
+// RUN: %clang_cc1 %s -DPEDANTIC -pedantic -fsyntax-only -verify
+
+#if PEDANTIC
+void g() {
+  if (true)
+[[likely]] {} // expected-warning {{use of the 'likely' attribute is a C++20 extension}}
+  else
+[[unlikely]] {} // expected-warning {{use of the 'unlikely' attribute is a C++20 extension}}
+}
+#else
+void a() {
+  if (true)
+[[likely]]; // expected-warning {{conflicting attributes 'likely' are ignored}}
+  else
+[[likely]]; // expected-note {{conflicting attribute is here}}
+}
+
+void b() {
+  if (true)
+[[unlikely]]; // expected-warning {{conflicting attributes 'unlikely' are ignored}}
+  else
+[[unlikely]]; // expected-note {{conflicting attribute is here}}
+}
+
+void c() {
+  if (true)
+[[likely]];
+}
+
+void d() {
+  if (true)
+[[unlikely]];
+}
+
+void g() {
+  if (true)
+[[likely]] {}
+  else
+[[unlikely]] {}
+}
+
+void h() {
+  if (true)
+[[likely]] {}
+  else {
+  }
+}
+
+void 

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 7 inline comments as done.
Mordante added inline comments.
Herald added a subscriber: danielkiss.



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

aaron.ballman wrote:
> Mordante wrote:
> > aaron.ballman wrote:
> > > Mordante wrote:
> > > > Mordante wrote:
> > > > > aaron.ballman wrote:
> > > > > > Mordante wrote:
> > > > > > > Mordante wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > Mordante wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > Hmm, I'm on the fence about specifying `201803` for these 
> > > > > > > > > > > attributes. Given that this is only the start of 
> > > > > > > > > > > supporting the attribute, do we want to claim it already 
> > > > > > > > > > > matches the standard's behavior? Or do we just want to 
> > > > > > > > > > > return `1` to signify that we understand this attribute 
> > > > > > > > > > > but we don't yet fully support it in common cases (such 
> > > > > > > > > > > as on labels in switch statements, etc)?
> > > > > > > > > > > 
> > > > > > > > > > > As another question, should we consider adding a C2x 
> > > > > > > > > > > spelling `[[clang::likely]]` and `[[clang::unlikely]]` to 
> > > > > > > > > > > add this functionality to C?
> > > > > > > > > > I was also somewhat on the fence, for now I'll change it to 
> > > > > > > > > > specify `1`.
> > > > > > > > > > 
> > > > > > > > > > I'll have a look at the C2x changes. Just curious do you 
> > > > > > > > > > know whether there's a proposal to add this to C2x?
> > > > > > > > > > I'll have a look at the C2x changes. Just curious do you 
> > > > > > > > > > know whether there's a proposal to add this to C2x?
> > > > > > > > > 
> > > > > > > > > There isn't one currently because there is no implementation 
> > > > > > > > > experience with the attributes in C. This is a way to get 
> > > > > > > > > that implementation experience so it's easier to propose the 
> > > > > > > > > feature to WG14.
> > > > > > > > > I was also somewhat on the fence, for now I'll change it to 
> > > > > > > > > specify `1`.
> > > > > > > > 
> > > > > > > > There seems to be an issue using the `1` so I used `2` instead. 
> > > > > > > > (Didn't want to look closely at the issue.)
> > > > > > > > 
> > > > > > > > > I'll have a look at the C2x changes. Just curious do you know 
> > > > > > > > > whether there's a proposal to add this to C2x?
> > > > > > > > 
> > > > > > > > There isn't one currently because there is no implementation 
> > > > > > > > experience with the attributes in C. This is a way to get that 
> > > > > > > > implementation experience so it's easier to propose the feature 
> > > > > > > > to WG14.
> > > > > > > 
> > > > > > > Nice to know. It seems the C2x wasn't at straightforward as I 
> > > > > > > hoped, so I didn't implement it yet. I intend to look at it 
> > > > > > > later. I first want the initial part done to see whether this is 
> > > > > > > the right approach.
> > > > > > What issues are you running into? 1 is the default value when you 
> > > > > > don't specify a value specifically.
> > > > > It give the error "clang/include/clang/Basic/Attr.td:265:7: error: 
> > > > > Standard attributes must have valid version information."
> > > > > > > I'll have a look at the C2x changes. Just curious do you know 
> > > > > > > whether there's a proposal to add this to C2x?
> > > > > > 
> > > > > > There isn't one currently because there is no implementation 
> > > > > > experience with the attributes in C. This is a way to get that 
> > > > > > implementation experience so it's easier to propose the feature to 
> > > > > > WG14.
> > > > > 
> > > > > Nice to know. It seems the C2x wasn't at straightforward as I hoped, 
> > > > > so I didn't implement it yet. I intend to look at it later. I first 
> > > > > want the initial part done to see whether this is the right approach.
> > > > 
> > > > I had another look and I got it working in C.
> > > If you leave the version number off entirely, it defaults to 1.
> > Yes and that gives the same error. So the default value doesn't "compile". 
> > I assume that's intentional to avoid setting a date of 1 for standard 
> > attributes. So we could keep it at 2 or switch back to `201803`. Which 
> > value do you prefer?
> Ah, yeah, you're right (sorry for the noise). I think `2` is fine for now 
> unless you find that the new patch actually hits enough of the important 
> cases from the standard to justify `201803`. We can figure that out with the 
> updated patch series.
For now let's keep it at `2`, this patch won't implement the `switch`. Once the 
`switch` works the iteration statements still need to be done, but I think they 
aren't as important as the selection statements.


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

https://reviews.llvm.org/D85091

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



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

Mordante wrote:
> aaron.ballman wrote:
> > Mordante wrote:
> > > Mordante wrote:
> > > > aaron.ballman wrote:
> > > > > Mordante wrote:
> > > > > > Mordante wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > Mordante wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > Hmm, I'm on the fence about specifying `201803` for these 
> > > > > > > > > > attributes. Given that this is only the start of supporting 
> > > > > > > > > > the attribute, do we want to claim it already matches the 
> > > > > > > > > > standard's behavior? Or do we just want to return `1` to 
> > > > > > > > > > signify that we understand this attribute but we don't yet 
> > > > > > > > > > fully support it in common cases (such as on labels in 
> > > > > > > > > > switch statements, etc)?
> > > > > > > > > > 
> > > > > > > > > > As another question, should we consider adding a C2x 
> > > > > > > > > > spelling `[[clang::likely]]` and `[[clang::unlikely]]` to 
> > > > > > > > > > add this functionality to C?
> > > > > > > > > I was also somewhat on the fence, for now I'll change it to 
> > > > > > > > > specify `1`.
> > > > > > > > > 
> > > > > > > > > I'll have a look at the C2x changes. Just curious do you know 
> > > > > > > > > whether there's a proposal to add this to C2x?
> > > > > > > > > I'll have a look at the C2x changes. Just curious do you know 
> > > > > > > > > whether there's a proposal to add this to C2x?
> > > > > > > > 
> > > > > > > > There isn't one currently because there is no implementation 
> > > > > > > > experience with the attributes in C. This is a way to get that 
> > > > > > > > implementation experience so it's easier to propose the feature 
> > > > > > > > to WG14.
> > > > > > > > I was also somewhat on the fence, for now I'll change it to 
> > > > > > > > specify `1`.
> > > > > > > 
> > > > > > > There seems to be an issue using the `1` so I used `2` instead. 
> > > > > > > (Didn't want to look closely at the issue.)
> > > > > > > 
> > > > > > > > I'll have a look at the C2x changes. Just curious do you know 
> > > > > > > > whether there's a proposal to add this to C2x?
> > > > > > > 
> > > > > > > There isn't one currently because there is no implementation 
> > > > > > > experience with the attributes in C. This is a way to get that 
> > > > > > > implementation experience so it's easier to propose the feature 
> > > > > > > to WG14.
> > > > > > 
> > > > > > Nice to know. It seems the C2x wasn't at straightforward as I 
> > > > > > hoped, so I didn't implement it yet. I intend to look at it later. 
> > > > > > I first want the initial part done to see whether this is the right 
> > > > > > approach.
> > > > > What issues are you running into? 1 is the default value when you 
> > > > > don't specify a value specifically.
> > > > It give the error "clang/include/clang/Basic/Attr.td:265:7: error: 
> > > > Standard attributes must have valid version information."
> > > > > > I'll have a look at the C2x changes. Just curious do you know 
> > > > > > whether there's a proposal to add this to C2x?
> > > > > 
> > > > > There isn't one currently because there is no implementation 
> > > > > experience with the attributes in C. This is a way to get that 
> > > > > implementation experience so it's easier to propose the feature to 
> > > > > WG14.
> > > > 
> > > > Nice to know. It seems the C2x wasn't at straightforward as I hoped, so 
> > > > I didn't implement it yet. I intend to look at it later. I first want 
> > > > the initial part done to see whether this is the right approach.
> > > 
> > > I had another look and I got it working in C.
> > If you leave the version number off entirely, it defaults to 1.
> Yes and that gives the same error. So the default value doesn't "compile". I 
> assume that's intentional to avoid setting a date of 1 for standard 
> attributes. So we could keep it at 2 or switch back to `201803`. Which value 
> do you prefer?
Ah, yeah, you're right (sorry for the noise). I think `2` is fine for now 
unless you find that the new patch actually hits enough of the important cases 
from the standard to justify `201803`. We can figure that out with the updated 
patch series.



Comment at: clang/include/clang/Basic/AttrDocs.td:1726
+  }
+  }];
+}

Mordante wrote:
> aaron.ballman wrote:
> > Mordante wrote:
> > > aaron.ballman wrote:
> > > > Something else we should document is what our behavior is when the 
> > > > attribute is not immediately inside of an `if` or `else` statement. 
> > > > e.g.,
> > > > ```
> > > > void func() { // Does not behave as though specified with [[gnu::hot]]
> > > >   [[likely]];
> > > > }
> > > > 
> > > > void func() {
> > > >   if (x) {
> > > > {
> > > >   [[likely]]; 

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Mark de Wever via Phabricator via cfe-commits]

Mordante planned changes to this revision.
Mordante marked 15 inline comments as done.
Mordante added a comment.

I fixed the issues locally. Before I resubmit them I'd rather discuss in D86559 
 the direction we want to take, so we can 
avoid an additional review cycle.




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

aaron.ballman wrote:
> Mordante wrote:
> > Mordante wrote:
> > > aaron.ballman wrote:
> > > > Mordante wrote:
> > > > > Mordante wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > Mordante wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > Hmm, I'm on the fence about specifying `201803` for these 
> > > > > > > > > attributes. Given that this is only the start of supporting 
> > > > > > > > > the attribute, do we want to claim it already matches the 
> > > > > > > > > standard's behavior? Or do we just want to return `1` to 
> > > > > > > > > signify that we understand this attribute but we don't yet 
> > > > > > > > > fully support it in common cases (such as on labels in switch 
> > > > > > > > > statements, etc)?
> > > > > > > > > 
> > > > > > > > > As another question, should we consider adding a C2x spelling 
> > > > > > > > > `[[clang::likely]]` and `[[clang::unlikely]]` to add this 
> > > > > > > > > functionality to C?
> > > > > > > > I was also somewhat on the fence, for now I'll change it to 
> > > > > > > > specify `1`.
> > > > > > > > 
> > > > > > > > I'll have a look at the C2x changes. Just curious do you know 
> > > > > > > > whether there's a proposal to add this to C2x?
> > > > > > > > I'll have a look at the C2x changes. Just curious do you know 
> > > > > > > > whether there's a proposal to add this to C2x?
> > > > > > > 
> > > > > > > There isn't one currently because there is no implementation 
> > > > > > > experience with the attributes in C. This is a way to get that 
> > > > > > > implementation experience so it's easier to propose the feature 
> > > > > > > to WG14.
> > > > > > > I was also somewhat on the fence, for now I'll change it to 
> > > > > > > specify `1`.
> > > > > > 
> > > > > > There seems to be an issue using the `1` so I used `2` instead. 
> > > > > > (Didn't want to look closely at the issue.)
> > > > > > 
> > > > > > > I'll have a look at the C2x changes. Just curious do you know 
> > > > > > > whether there's a proposal to add this to C2x?
> > > > > > 
> > > > > > There isn't one currently because there is no implementation 
> > > > > > experience with the attributes in C. This is a way to get that 
> > > > > > implementation experience so it's easier to propose the feature to 
> > > > > > WG14.
> > > > > 
> > > > > Nice to know. It seems the C2x wasn't at straightforward as I hoped, 
> > > > > so I didn't implement it yet. I intend to look at it later. I first 
> > > > > want the initial part done to see whether this is the right approach.
> > > > What issues are you running into? 1 is the default value when you don't 
> > > > specify a value specifically.
> > > It give the error "clang/include/clang/Basic/Attr.td:265:7: error: 
> > > Standard attributes must have valid version information."
> > > > > I'll have a look at the C2x changes. Just curious do you know whether 
> > > > > there's a proposal to add this to C2x?
> > > > 
> > > > There isn't one currently because there is no implementation experience 
> > > > with the attributes in C. This is a way to get that implementation 
> > > > experience so it's easier to propose the feature to WG14.
> > > 
> > > Nice to know. It seems the C2x wasn't at straightforward as I hoped, so I 
> > > didn't implement it yet. I intend to look at it later. I first want the 
> > > initial part done to see whether this is the right approach.
> > 
> > I had another look and I got it working in C.
> If you leave the version number off entirely, it defaults to 1.
Yes and that gives the same error. So the default value doesn't "compile". I 
assume that's intentional to avoid setting a date of 1 for standard attributes. 
So we could keep it at 2 or switch back to `201803`. Which value do you prefer?



Comment at: clang/include/clang/Basic/AttrDocs.td:1691
+The ``likely`` and ``unlikely`` attributes are used as compiler hints. When
+the next executed statement depends on a condition this attribute can
+annotate all possible statements with either ``likely`` or ``unlikely``.

aaron.ballman wrote:
> I think this paragraph is misleading because the attribute no longer impacts 
> the statement, it impacts the *entire branch the statement is contained 
> within*.
I'll update the documentation including the other remarks added.



Comment at: clang/include/clang/Basic/AttrDocs.td:1726
+  }
+  }];
+}

aaron.ballman wrote:
> Mordante wrote:
> > aaron.ballman 

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1726
+  }
+  }];
+}

Mordante wrote:
> aaron.ballman wrote:
> > Something else we should document is what our behavior is when the 
> > attribute is not immediately inside of an `if` or `else` statement. e.g.,
> > ```
> > void func() { // Does not behave as though specified with [[gnu::hot]]
> >   [[likely]];
> > }
> > 
> > void func() {
> >   if (x) {
> > {
> >   [[likely]]; // Does this make the if branch likely?
> >   SomeRAIIObj Obj;
> > }
> >   } else {
> >   }
> > }
> > 
> > void func() {
> >   if (x) {
> > int y = ({[[likely]]; /* Does this make the if branch likely? */ 1;});
> >   } else {
> >   }
> > }
> > ```
> > Something else we should document is what our behavior is when the 
> > attribute is not immediately inside of an `if` or `else` statement. e.g.,
> > ```
> > void func() { // Does not behave as though specified with [[gnu::hot]]
> >   [[likely]];
> > }
> 
> No a few days ago I wondered whether it makes sense, but the [[gnu::hot]] 
> should be at the declaration of the function and not in the body. 
> So I think this doesn't make sense and the `[[likely]]` here will be ignored.
> 
> > void func() {
> >   if (x) {
> > {
> >   [[likely]]; // Does this make the if branch likely?
> >   SomeRAIIObj Obj;
> > }
> >   } else {
> >   }
> > }
> 
> Yes this should work, the attribute will recursively visit compound 
> statements.
> 
> > void func() {
> >   if (x) {
> > int y = ({[[likely]]; /* Does this make the if branch likely? */ 1;});
> >   } else {
> >   }
> > }
> > ```
> 
> Not in this patch. I'm working on more improvements to make switch statements 
> working. I tested it with my current WIP and there it does work.
> So in the future this will work.
> So I think this doesn't make sense and the [[likely]] here will be ignored.

Great, that matches the behavior I was hoping for.

> Yes this should work, the attribute will recursively visit compound 
> statements.

Great!

> Not in this patch. I'm working on more improvements to make switch statements 
> working.

I'm not certain what that example had to do with switch statement, but just as 
an FYI, I'd like this to work initially (assuming it's not overly hard) only 
because GNU statement expressions show up rather frequently in macros as an 
alternative form of the `do ... while (0);` pattern, which can have 
likely/unlikely path sensitivity.



Comment at: clang/lib/CodeGen/CGStmt.cpp:691
+  Visit(S);
+  if (Result != None)
+return;

Mordante wrote:
> aaron.ballman wrote:
> > Is early return the correct logic? Won't that give surprising results in 
> > the case the programmer has both attributes in the same compound statement? 
> > I think we need to look over all the statements in the current block, 
> > increment a counter when we hit `[[likely]]`, decrement the counter when we 
> > hit `[[unlikely]]` and return whether the counter is 0, negative 
> > (unlikely), or positive (likely).
> Yes here it accepts the first likelihood it finds and accepts that as the 
> wanted likelihood. I also start to doubt whether that's wanted. In my current 
> switch WIP I  issue an diagnostic if there's a conflict between the 
> likelihood diagnostics and ignore them. I feel that's a better approach. This 
> diagnostic is added to the `-Wignored-attributes` group, which is shown by 
> default.
> 
> I don't like the idea of using a counter. If the attribute is "hidden" in a 
> validation macro, adding it to a branch might suddenly change the likelihood 
> of that branch.
> 
> I don't like the idea of using a counter. If the attribute is "hidden" in a 
> validation macro, adding it to a branch might suddenly change the likelihood 
> of that branch.

That's definitely true and is also a concern of mine. I don't know what the 
correct answer is here by going on a per-statement basis. None of the solutions 
are satisfying and all of them leave this feature with surprising behavior.



Comment at: clang/lib/CodeGen/CGStmt.cpp:791
+  llvm::MDNode *Weights = nullptr;
+  uint64_t Count = getProfileCount(S.getThen());
+  if (!Count && CGM.getCodeGenOpts().OptimizationLevel) {

Mordante wrote:
> aaron.ballman wrote:
> > Perhaps not for this patch, but I wonder if we'd be doing users a good deed 
> > by alerting them when PGO weights do not match the attribute specified. 
> > e.g., if an attribute says "this branch is likely" and PGO shows it's 
> > unlikely, that seems like something the programmer may wish to know. WDYT?
> I already investigated before and there's a diagnostic 
> `warn_profile_data_misexpect` when using `__builtin_expect` so I expect I can 
> reuse existing code. So I want to have a look at it, but I first want to get 
> the more important parts of the likelihood attributes working properly.
> So I want to have 

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 4 inline comments as done.
Mordante added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1726
+  }
+  }];
+}

aaron.ballman wrote:
> Something else we should document is what our behavior is when the attribute 
> is not immediately inside of an `if` or `else` statement. e.g.,
> ```
> void func() { // Does not behave as though specified with [[gnu::hot]]
>   [[likely]];
> }
> 
> void func() {
>   if (x) {
> {
>   [[likely]]; // Does this make the if branch likely?
>   SomeRAIIObj Obj;
> }
>   } else {
>   }
> }
> 
> void func() {
>   if (x) {
> int y = ({[[likely]]; /* Does this make the if branch likely? */ 1;});
>   } else {
>   }
> }
> ```
> Something else we should document is what our behavior is when the attribute 
> is not immediately inside of an `if` or `else` statement. e.g.,
> ```
> void func() { // Does not behave as though specified with [[gnu::hot]]
>   [[likely]];
> }

No a few days ago I wondered whether it makes sense, but the [[gnu::hot]] 
should be at the declaration of the function and not in the body. 
So I think this doesn't make sense and the `[[likely]]` here will be ignored.

> void func() {
>   if (x) {
> {
>   [[likely]]; // Does this make the if branch likely?
>   SomeRAIIObj Obj;
> }
>   } else {
>   }
> }

Yes this should work, the attribute will recursively visit compound statements.

> void func() {
>   if (x) {
> int y = ({[[likely]]; /* Does this make the if branch likely? */ 1;});
>   } else {
>   }
> }
> ```

Not in this patch. I'm working on more improvements to make switch statements 
working. I tested it with my current WIP and there it does work.
So in the future this will work.



Comment at: clang/lib/CodeGen/CGStmt.cpp:691
+  Visit(S);
+  if (Result != None)
+return;

aaron.ballman wrote:
> Is early return the correct logic? Won't that give surprising results in the 
> case the programmer has both attributes in the same compound statement? I 
> think we need to look over all the statements in the current block, increment 
> a counter when we hit `[[likely]]`, decrement the counter when we hit 
> `[[unlikely]]` and return whether the counter is 0, negative (unlikely), or 
> positive (likely).
Yes here it accepts the first likelihood it finds and accepts that as the 
wanted likelihood. I also start to doubt whether that's wanted. In my current 
switch WIP I  issue an diagnostic if there's a conflict between the likelihood 
diagnostics and ignore them. I feel that's a better approach. This diagnostic 
is added to the `-Wignored-attributes` group, which is shown by default.

I don't like the idea of using a counter. If the attribute is "hidden" in a 
validation macro, adding it to a branch might suddenly change the likelihood of 
that branch.




Comment at: clang/lib/CodeGen/CGStmt.cpp:723
+  // The user can use conflicting likelihood attributes within one of the
+  // statements or between the statements. These conflicts are ignored and the
+  // first match is used.

aaron.ballman wrote:
> I do not think conflicts should result in the first match being used (unless 
> we're diagnosing the situation, at which point some of this code should be 
> lifted into Sema and set a bit on an AST node that can be read during codegen 
> time), so this comment may need to be updated.
Agreed. In my current switch WIP I already started experimenting with moving 
the likelihood selection to Sema and store the likelihood in the AST bits.



Comment at: clang/lib/CodeGen/CGStmt.cpp:791
+  llvm::MDNode *Weights = nullptr;
+  uint64_t Count = getProfileCount(S.getThen());
+  if (!Count && CGM.getCodeGenOpts().OptimizationLevel) {

aaron.ballman wrote:
> Perhaps not for this patch, but I wonder if we'd be doing users a good deed 
> by alerting them when PGO weights do not match the attribute specified. e.g., 
> if an attribute says "this branch is likely" and PGO shows it's unlikely, 
> that seems like something the programmer may wish to know. WDYT?
I already investigated before and there's a diagnostic 
`warn_profile_data_misexpect` when using `__builtin_expect` so I expect I can 
reuse existing code. So I want to have a look at it, but I first want to get 
the more important parts of the likelihood attributes working properly.


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

https://reviews.llvm.org/D85091

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D85091#2232588 , @Mordante wrote:

> The current implementation matches GCC's behaviour. This behaviour aligns 
> best with my interpretation of the standard and GCC seems to have the most 
> mature implementation. I would like to get the current implementation in 
> Clang and proceed to work on the missing pieces. In parallel we can align 
> with the other vendors and, if needed, adjust our implementation.

I agree with this direction.




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

Mordante wrote:
> Mordante wrote:
> > aaron.ballman wrote:
> > > Mordante wrote:
> > > > Mordante wrote:
> > > > > aaron.ballman wrote:
> > > > > > Mordante wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > Hmm, I'm on the fence about specifying `201803` for these 
> > > > > > > > attributes. Given that this is only the start of supporting the 
> > > > > > > > attribute, do we want to claim it already matches the 
> > > > > > > > standard's behavior? Or do we just want to return `1` to 
> > > > > > > > signify that we understand this attribute but we don't yet 
> > > > > > > > fully support it in common cases (such as on labels in switch 
> > > > > > > > statements, etc)?
> > > > > > > > 
> > > > > > > > As another question, should we consider adding a C2x spelling 
> > > > > > > > `[[clang::likely]]` and `[[clang::unlikely]]` to add this 
> > > > > > > > functionality to C?
> > > > > > > I was also somewhat on the fence, for now I'll change it to 
> > > > > > > specify `1`.
> > > > > > > 
> > > > > > > I'll have a look at the C2x changes. Just curious do you know 
> > > > > > > whether there's a proposal to add this to C2x?
> > > > > > > I'll have a look at the C2x changes. Just curious do you know 
> > > > > > > whether there's a proposal to add this to C2x?
> > > > > > 
> > > > > > There isn't one currently because there is no implementation 
> > > > > > experience with the attributes in C. This is a way to get that 
> > > > > > implementation experience so it's easier to propose the feature to 
> > > > > > WG14.
> > > > > > I was also somewhat on the fence, for now I'll change it to specify 
> > > > > > `1`.
> > > > > 
> > > > > There seems to be an issue using the `1` so I used `2` instead. 
> > > > > (Didn't want to look closely at the issue.)
> > > > > 
> > > > > > I'll have a look at the C2x changes. Just curious do you know 
> > > > > > whether there's a proposal to add this to C2x?
> > > > > 
> > > > > There isn't one currently because there is no implementation 
> > > > > experience with the attributes in C. This is a way to get that 
> > > > > implementation experience so it's easier to propose the feature to 
> > > > > WG14.
> > > > 
> > > > Nice to know. It seems the C2x wasn't at straightforward as I hoped, so 
> > > > I didn't implement it yet. I intend to look at it later. I first want 
> > > > the initial part done to see whether this is the right approach.
> > > What issues are you running into? 1 is the default value when you don't 
> > > specify a value specifically.
> > It give the error "clang/include/clang/Basic/Attr.td:265:7: error: Standard 
> > attributes must have valid version information."
> > > > I'll have a look at the C2x changes. Just curious do you know whether 
> > > > there's a proposal to add this to C2x?
> > > 
> > > There isn't one currently because there is no implementation experience 
> > > with the attributes in C. This is a way to get that implementation 
> > > experience so it's easier to propose the feature to WG14.
> > 
> > Nice to know. It seems the C2x wasn't at straightforward as I hoped, so I 
> > didn't implement it yet. I intend to look at it later. I first want the 
> > initial part done to see whether this is the right approach.
> 
> I had another look and I got it working in C.
If you leave the version number off entirely, it defaults to 1.



Comment at: clang/include/clang/Basic/AttrDocs.td:1691
+The ``likely`` and ``unlikely`` attributes are used as compiler hints. When
+the next executed statement depends on a condition this attribute can
+annotate all possible statements with either ``likely`` or ``unlikely``.

I think this paragraph is misleading because the attribute no longer impacts 
the statement, it impacts the *entire branch the statement is contained within*.



Comment at: clang/include/clang/Basic/AttrDocs.td:1696
+
+It's not allowed to annotate a statement with both ``likely`` and
+``unlikely``.

You should clarify here that you CAN annotate multiple statements in the same 
block and what we do about it. e.g.,
```
if (foo) {
  [[likely]];
  [[unlikely]];
  bar();
}
```
Note, I doubt this will happen as obviously as this example. More likely what 
will 

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Mark de Wever via Phabricator via cfe-commits]

Mordante updated this revision to Diff 287264.
Mordante marked 2 inline comments as done.
Mordante added a comment.

Addresses review comments:

- Adds support for c2x using `[[clang::likely]]` and `[[clang::unlikely]]`
- Remove warnings about conflicting likelihoods, except those required by the 
Standard
- The attributes can now be placed inside the compound statement in of the 
`ThenStmt` and `ElseStmt`

The current implementation matches GCC's behaviour. This behaviour aligns best 
with my interpretation of the standard and GCC seems to have the most mature 
implementation. I would like to get the current implementation in Clang and 
proceed to work on the missing pieces. In parallel we can align with the other 
vendors and, if needed, adjust our implementation.


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

https://reviews.llvm.org/D85091

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Parse/ParseDeclCXX.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGenCXX/attr-likelihood-if-branch-weights.cpp
  clang/test/Preprocessor/has_attribute.cpp
  clang/test/Sema/attr-likelihood.c
  clang/test/SemaCXX/attr-likelihood.cpp
  clang/www/cxx_status.html
  llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp

Index: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
===
--- llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
+++ llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
@@ -24,7 +24,6 @@
 #include "llvm/IR/Metadata.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
-#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Utils/MisExpect.h"
@@ -48,10 +47,10 @@
 // 'select' instructions. It may be worthwhile to hoist these values to some
 // shared space, so they can be used directly by other passes.
 
-static cl::opt LikelyBranchWeight(
+cl::opt llvm::LikelyBranchWeight(
 "likely-branch-weight", cl::Hidden, cl::init(2000),
 cl::desc("Weight of the branch likely to be taken (default = 2000)"));
-static cl::opt UnlikelyBranchWeight(
+cl::opt llvm::UnlikelyBranchWeight(
 "unlikely-branch-weight", cl::Hidden, cl::init(1),
 cl::desc("Weight of the branch unlikely to be taken (default = 1)"));
 
Index: llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
===
--- llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
+++ llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
@@ -17,6 +17,7 @@
 
 #include "llvm/IR/Function.h"
 #include "llvm/IR/PassManager.h"
+#include "llvm/Support/CommandLine.h"
 
 namespace llvm {
 
@@ -31,6 +32,8 @@
   PreservedAnalyses run(Function , FunctionAnalysisManager &);
 };
 
+extern cl::opt LikelyBranchWeight;
+extern cl::opt UnlikelyBranchWeight;
 }
 
 #endif
Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -987,7 +987,7 @@
 
   [[likely]] and [[unlikely]] attributes
   https://wg21.link/p0479r5;>P0479R5
-  No
+  Clang 12 (partial)
 
 
   typename optional in more contexts
Index: clang/test/SemaCXX/attr-likelihood.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-likelihood.cpp
@@ -0,0 +1,132 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify
+// RUN: %clang_cc1 %s -DPEDANTIC -pedantic -fsyntax-only -verify
+
+#if PEDANTIC
+void g() {
+  if (true)
+[[likely]] {} // expected-warning {{use of the 'likely' attribute is a C++20 extension}}
+  else
+[[unlikely]] {} // expected-warning {{use of the 'unlikely' attribute is a C++20 extension}}
+}
+#else
+void a() {
+  if (true)
+[[likely]]   // expected-note {{conflicting attribute is here}}
+[[unlikely]] // expected-error {{unlikely and likely attributes are not compatible}}
+;
+}
+
+void b() {
+  if (true)
+[[unlikely]] // expected-note {{conflicting attribute is here}}
+[[likely]]   // expected-error {{likely and unlikely attributes are not compatible}}
+;
+}
+
+void c() {
+  if (true)
+[[likely]];
+}
+
+void d() {
+  if (true)
+[[unlikely]];
+}
+
+void g() {
+  if (true)
+[[likely]] {}
+  else
+[[unlikely]] {}
+}
+
+void h() {
+  if (true)
+[[likely]] {}
+  else {
+  }
+}
+
+void i() {
+  if (true)
+[[unlikely]] {}
+  else {
+  }
+}
+
+void j() {
+  if (true) {
+  } else
+[[likely]] {}
+}
+
+void k() {
+  if (true) {
+  } else
+[[likely]] {}
+}
+
+void l() {
+  if (true)
+[[likely]] {}
+  else
+[[unlikely]] if (false) [[likely]] {}
+}
+
+void m() {
+  [[likely]] int x = 42; 

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 8 inline comments as done.
Mordante added inline comments.



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

aaron.ballman wrote:
> Mordante wrote:
> > Mordante wrote:
> > > aaron.ballman wrote:
> > > > Mordante wrote:
> > > > > aaron.ballman wrote:
> > > > > > Hmm, I'm on the fence about specifying `201803` for these 
> > > > > > attributes. Given that this is only the start of supporting the 
> > > > > > attribute, do we want to claim it already matches the standard's 
> > > > > > behavior? Or do we just want to return `1` to signify that we 
> > > > > > understand this attribute but we don't yet fully support it in 
> > > > > > common cases (such as on labels in switch statements, etc)?
> > > > > > 
> > > > > > As another question, should we consider adding a C2x spelling 
> > > > > > `[[clang::likely]]` and `[[clang::unlikely]]` to add this 
> > > > > > functionality to C?
> > > > > I was also somewhat on the fence, for now I'll change it to specify 
> > > > > `1`.
> > > > > 
> > > > > I'll have a look at the C2x changes. Just curious do you know whether 
> > > > > there's a proposal to add this to C2x?
> > > > > I'll have a look at the C2x changes. Just curious do you know whether 
> > > > > there's a proposal to add this to C2x?
> > > > 
> > > > There isn't one currently because there is no implementation experience 
> > > > with the attributes in C. This is a way to get that implementation 
> > > > experience so it's easier to propose the feature to WG14.
> > > > I was also somewhat on the fence, for now I'll change it to specify `1`.
> > > 
> > > There seems to be an issue using the `1` so I used `2` instead. (Didn't 
> > > want to look closely at the issue.)
> > > 
> > > > I'll have a look at the C2x changes. Just curious do you know whether 
> > > > there's a proposal to add this to C2x?
> > > 
> > > There isn't one currently because there is no implementation experience 
> > > with the attributes in C. This is a way to get that implementation 
> > > experience so it's easier to propose the feature to WG14.
> > 
> > Nice to know. It seems the C2x wasn't at straightforward as I hoped, so I 
> > didn't implement it yet. I intend to look at it later. I first want the 
> > initial part done to see whether this is the right approach.
> What issues are you running into? 1 is the default value when you don't 
> specify a value specifically.
It give the error "clang/include/clang/Basic/Attr.td:265:7: error: Standard 
attributes must have valid version information."



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

Mordante wrote:
> aaron.ballman wrote:
> > Mordante wrote:
> > > Mordante wrote:
> > > > aaron.ballman wrote:
> > > > > Mordante wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > Hmm, I'm on the fence about specifying `201803` for these 
> > > > > > > attributes. Given that this is only the start of supporting the 
> > > > > > > attribute, do we want to claim it already matches the standard's 
> > > > > > > behavior? Or do we just want to return `1` to signify that we 
> > > > > > > understand this attribute but we don't yet fully support it in 
> > > > > > > common cases (such as on labels in switch statements, etc)?
> > > > > > > 
> > > > > > > As another question, should we consider adding a C2x spelling 
> > > > > > > `[[clang::likely]]` and `[[clang::unlikely]]` to add this 
> > > > > > > functionality to C?
> > > > > > I was also somewhat on the fence, for now I'll change it to specify 
> > > > > > `1`.
> > > > > > 
> > > > > > I'll have a look at the C2x changes. Just curious do you know 
> > > > > > whether there's a proposal to add this to C2x?
> > > > > > I'll have a look at the C2x changes. Just curious do you know 
> > > > > > whether there's a proposal to add this to C2x?
> > > > > 
> > > > > There isn't one currently because there is no implementation 
> > > > > experience with the attributes in C. This is a way to get that 
> > > > > implementation experience so it's easier to propose the feature to 
> > > > > WG14.
> > > > > I was also somewhat on the fence, for now I'll change it to specify 
> > > > > `1`.
> > > > 
> > > > There seems to be an issue using the `1` so I used `2` instead. (Didn't 
> > > > want to look closely at the issue.)
> > > > 
> > > > > I'll have a look at the C2x changes. Just curious do you know whether 
> > > > > there's a proposal to add this to C2x?
> > > > 
> > > > There isn't one currently because there is no implementation experience 
> > > > with the attributes in C. This is a way to get that implementation 
> > > > experience so it's easier to propose the feature to WG14.
> > > 
> > > Nice to know. It seems the C2x wasn't at 

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/SemaCXX/attr-likelihood.cpp:101
+}
+#endif

Quuxplusone wrote:
> aaron.ballman wrote:
> > Mordante wrote:
> > > aaron.ballman wrote:
> > > > Mordante wrote:
> > > > > Quuxplusone wrote:
> > > > > > I'd like to see a case like `if (x) [[likely]] i=1;` just to prove 
> > > > > > that it works on statements that aren't blocks or empty statements. 
> > > > > > (My understanding is that this should already work with your 
> > > > > > current patch.)
> > > > > > 
> > > > > > I'd like to see a case like `if (x) { [[likely]] i=1; }` to prove 
> > > > > > that it works on arbitrary statements. This //should// have the 
> > > > > > same effect as `if (x) [[likely]] { i=1; }`. My understanding is 
> > > > > > that your current patch doesn't get us there //yet//. If it's 
> > > > > > unclear how we'd get there by proceeding along your current 
> > > > > > trajectory, then I would question whether we want to commit to this 
> > > > > > trajectory at all, yet.
> > > > > I agree it would be a good idea to add a test like `if (x) [[likely]] 
> > > > > i=1;` but I don't feel adding an additional test in this file proves 
> > > > > anything. I'll add a test to 
> > > > > `clang/test/CodeGenCXX/attr-likelihood-if-branch-weights.cpp` to 
> > > > > prove it not only accepts the code but also honours the attribute. 
> > > > > This is especially important due to your correct observation that `if 
> > > > > (x) { [[likely]] i=1; }` doesn't have any effect. The code is 
> > > > > accepted but the CodeGen won't honour the attribute.
> > > > > 
> > > > > I think we can use the current approach, but I need to adjust 
> > > > > `getLikelihood`. Instead of only testing whether the current `Stmt` 
> > > > > is an `AttributedStmt` it needs to iterate over all `Stmt`s and test 
> > > > > them for the attribute. Obviously it should avoid looking into 
> > > > > `Stmt`s that also use the attribute, e.g:
> > > > > ```
> > > > > if(b) {
> > > > > switch(c) {
> > > > > [[unlikely]] case 0: ... break; // The attribute "belongs" to 
> > > > > the switch not to the if
> > > > > [[likely]] case 1: ... break; // The attribute "belongs" to 
> > > > > the switch not to the if
> > > > > }
> > > > > }
> > > > > ```
> > > > > 
> > > > > This is especially important due to your correct observation that if 
> > > > > (x) { [[likely]] i=1; } doesn't have any effect. 
> > > > 
> > > > This code should diagnose the attribute as being ignored.
> > > What I understood from Arthur and rereading the standard this should 
> > > work, but the initial version of the patch didn't handle this properly.
> > > What I understood from Arthur and rereading the standard this should 
> > > work, but the initial version of the patch didn't handle this properly.
> > 
> > My original belief was  that it's acceptable for the attribute to be placed 
> > there in terms of syntax, but the recommended practice doesn't apply to 
> > this case because there's no alternative paths of execution once you've 
> > entered the compound statement. That means:
> > ```
> > if (x) [[likely]] { i = 1; }
> > // is not the same as
> > if (x) { [[likely]] i = 1; }
> > ```
> > That said, I can squint at the words to get your interpretation and your 
> > interpretation matches what's in the original paper 
> > (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0479r2.html#examples).
> > 
> > Ultimately, I think we should strive for consistency between 
> > implementations, and I think we should file an issue with WG21 to clarify 
> > the wording once we figure out how implementations want to interpret 
> > questions like these.
> I don't know WG21's actual rationale, but I can imagine a programmer wanting 
> to annotate something like this:
> 
> #define FATAL(msg) [[unlikely]] log_and_abort("Fatal error!", msg);
> ...
> auto packet = receive();
> if (packet.malformed()) {
> save_to_log(packet);
> FATAL("malformed packet");
> }
> ...
> 
> The "absolute unlikeliness" of the malformed-packet codepath is encapsulated 
> within the `FATAL` macro so that the programmer doesn't have to tediously 
> repeat `[[unlikely]]` at some branch arbitrarily far above the `FATAL` point. 
> Compilers actually do this already, every time they see a `throw` — every 
> `throw` is implicitly "unlikely" and taints its whole codepath. We want to do 
> something similar for `[[unlikely]]`.
> 
> It's definitely going to be unsatisfyingly fuzzy logic, though, similar to 
> how inlining heuristics and `inline` are today.
> 
> My understanding is that
> 
> if (x) { [[likely]] i = 1; }
> if (x) [[likely]] { i = 1; }
> 
> have exactly the same surface reading: the same set of codepaths exist in 
> both cases, and the same "x true, i gets 1" codepath is `[[likely]]` in both 
> cases. Of course your heuristic might //choose// to treat them differently, 
> just 

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: clang/test/SemaCXX/attr-likelihood.cpp:101
+}
+#endif

aaron.ballman wrote:
> Mordante wrote:
> > aaron.ballman wrote:
> > > Mordante wrote:
> > > > Quuxplusone wrote:
> > > > > I'd like to see a case like `if (x) [[likely]] i=1;` just to prove 
> > > > > that it works on statements that aren't blocks or empty statements. 
> > > > > (My understanding is that this should already work with your current 
> > > > > patch.)
> > > > > 
> > > > > I'd like to see a case like `if (x) { [[likely]] i=1; }` to prove 
> > > > > that it works on arbitrary statements. This //should// have the same 
> > > > > effect as `if (x) [[likely]] { i=1; }`. My understanding is that your 
> > > > > current patch doesn't get us there //yet//. If it's unclear how we'd 
> > > > > get there by proceeding along your current trajectory, then I would 
> > > > > question whether we want to commit to this trajectory at all, yet.
> > > > I agree it would be a good idea to add a test like `if (x) [[likely]] 
> > > > i=1;` but I don't feel adding an additional test in this file proves 
> > > > anything. I'll add a test to 
> > > > `clang/test/CodeGenCXX/attr-likelihood-if-branch-weights.cpp` to prove 
> > > > it not only accepts the code but also honours the attribute. This is 
> > > > especially important due to your correct observation that `if (x) { 
> > > > [[likely]] i=1; }` doesn't have any effect. The code is accepted but 
> > > > the CodeGen won't honour the attribute.
> > > > 
> > > > I think we can use the current approach, but I need to adjust 
> > > > `getLikelihood`. Instead of only testing whether the current `Stmt` is 
> > > > an `AttributedStmt` it needs to iterate over all `Stmt`s and test them 
> > > > for the attribute. Obviously it should avoid looking into `Stmt`s that 
> > > > also use the attribute, e.g:
> > > > ```
> > > > if(b) {
> > > > switch(c) {
> > > > [[unlikely]] case 0: ... break; // The attribute "belongs" to 
> > > > the switch not to the if
> > > > [[likely]] case 1: ... break; // The attribute "belongs" to the 
> > > > switch not to the if
> > > > }
> > > > }
> > > > ```
> > > > 
> > > > This is especially important due to your correct observation that if 
> > > > (x) { [[likely]] i=1; } doesn't have any effect. 
> > > 
> > > This code should diagnose the attribute as being ignored.
> > What I understood from Arthur and rereading the standard this should work, 
> > but the initial version of the patch didn't handle this properly.
> > What I understood from Arthur and rereading the standard this should work, 
> > but the initial version of the patch didn't handle this properly.
> 
> My original belief was  that it's acceptable for the attribute to be placed 
> there in terms of syntax, but the recommended practice doesn't apply to this 
> case because there's no alternative paths of execution once you've entered 
> the compound statement. That means:
> ```
> if (x) [[likely]] { i = 1; }
> // is not the same as
> if (x) { [[likely]] i = 1; }
> ```
> That said, I can squint at the words to get your interpretation and your 
> interpretation matches what's in the original paper 
> (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0479r2.html#examples).
> 
> Ultimately, I think we should strive for consistency between implementations, 
> and I think we should file an issue with WG21 to clarify the wording once we 
> figure out how implementations want to interpret questions like these.
I don't know WG21's actual rationale, but I can imagine a programmer wanting to 
annotate something like this:

#define FATAL(msg) [[unlikely]] log_and_abort("Fatal error!", msg);
...
auto packet = receive();
if (packet.malformed()) {
save_to_log(packet);
FATAL("malformed packet");
}
...

The "absolute unlikeliness" of the malformed-packet codepath is encapsulated 
within the `FATAL` macro so that the programmer doesn't have to tediously 
repeat `[[unlikely]]` at some branch arbitrarily far above the `FATAL` point. 
Compilers actually do this already, every time they see a `throw` — every 
`throw` is implicitly "unlikely" and taints its whole codepath. We want to do 
something similar for `[[unlikely]]`.

It's definitely going to be unsatisfyingly fuzzy logic, though, similar to how 
inlining heuristics and `inline` are today.

My understanding is that

if (x) { [[likely]] i = 1; }
if (x) [[likely]] { i = 1; }

have exactly the same surface reading: the same set of codepaths exist in both 
cases, and the same "x true, i gets 1" codepath is `[[likely]]` in both cases. 
Of course your heuristic might //choose// to treat them differently, just as 
you might //choose// to treat

struct S { int f() { ... } };
struct S { inline int f() { ... } };

differently for inlining purposes despite their identical surface readings.


Repository:
  rG LLVM Github Monorepo

CHANGES 

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



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

Mordante wrote:
> Mordante wrote:
> > aaron.ballman wrote:
> > > Mordante wrote:
> > > > aaron.ballman wrote:
> > > > > Hmm, I'm on the fence about specifying `201803` for these attributes. 
> > > > > Given that this is only the start of supporting the attribute, do we 
> > > > > want to claim it already matches the standard's behavior? Or do we 
> > > > > just want to return `1` to signify that we understand this attribute 
> > > > > but we don't yet fully support it in common cases (such as on labels 
> > > > > in switch statements, etc)?
> > > > > 
> > > > > As another question, should we consider adding a C2x spelling 
> > > > > `[[clang::likely]]` and `[[clang::unlikely]]` to add this 
> > > > > functionality to C?
> > > > I was also somewhat on the fence, for now I'll change it to specify `1`.
> > > > 
> > > > I'll have a look at the C2x changes. Just curious do you know whether 
> > > > there's a proposal to add this to C2x?
> > > > I'll have a look at the C2x changes. Just curious do you know whether 
> > > > there's a proposal to add this to C2x?
> > > 
> > > There isn't one currently because there is no implementation experience 
> > > with the attributes in C. This is a way to get that implementation 
> > > experience so it's easier to propose the feature to WG14.
> > > I was also somewhat on the fence, for now I'll change it to specify `1`.
> > 
> > There seems to be an issue using the `1` so I used `2` instead. (Didn't 
> > want to look closely at the issue.)
> > 
> > > I'll have a look at the C2x changes. Just curious do you know whether 
> > > there's a proposal to add this to C2x?
> > 
> > There isn't one currently because there is no implementation experience 
> > with the attributes in C. This is a way to get that implementation 
> > experience so it's easier to propose the feature to WG14.
> 
> Nice to know. It seems the C2x wasn't at straightforward as I hoped, so I 
> didn't implement it yet. I intend to look at it later. I first want the 
> initial part done to see whether this is the right approach.
What issues are you running into? 1 is the default value when you don't specify 
a value specifically.



Comment at: clang/include/clang/Basic/AttrDocs.td:1697
+It's not allowed to annotate a statement with both ``likely`` and
+``unlikely``.  It's not recommended to annotate both branches of an ``if``
+statement with an attribute.

Mordante wrote:
> Mordante wrote:
> > aaron.ballman wrote:
> > > Mordante wrote:
> > > > Quuxplusone wrote:
> > > > > aaron.ballman wrote:
> > > > > > Why? I would expect this to be reasonable code:
> > > > > > ```
> > > > > > if (foo) [[likely]] {
> > > > > >   ...
> > > > > > } else if (bar) [[unlikely]] {
> > > > > >   ...
> > > > > > } else if (baz) [[likely]] {
> > > > > >   ...
> > > > > > } else {
> > > > > >   ...
> > > > > > }
> > > > > > ```
> > > > > > Especially because I would expect this to be reasonable code:
> > > > > > ```
> > > > > > switch (value) {
> > > > > > [[likely]] case 0: ... break;
> > > > > > [[unlikely]] case 1: ... break;
> > > > > > [[likely]] case 2: ... break;
> > > > > > [[unlikely]] default: ... break;
> > > > > > }
> > > > > > ```
> > > > > > As motivating examples, consider a code generator that knows 
> > > > > > whether a particular branch is likely or not and it writes out the 
> > > > > > attribute on all branches. Or, something like this:
> > > > > > ```
> > > > > > float rnd_value = get_super_random_number_between_zero_and_one();
> > > > > > if (rnd_value < .1) [[unlikely]] {
> > > > > > } else if (rnd_value > .9) [[unlikely]] {
> > > > > > } else [[likely]] {
> > > > > > }
> > > > > > ```
> > > > > Right, annotating both/multiple branches of a control statement 
> > > > > should be perfectly fine. Even `if (x) [[likely]] { } else [[likely]] 
> > > > > { }` should be perfectly okay as far as the code generator is 
> > > > > concerned (and we should have a test for that); it's silly, but 
> > > > > there's no reason to warn against it in the compiler docs.
> > > > > 
> > > > > Aaron, notice that `if (x) [[likely]] { } else if (y) [[likely]] { }` 
> > > > > is not actually annotating "both" branches of any single 
> > > > > `if`-statement. There you're annotating the true branch of an if 
> > > > > (without annotating the else branch), and then annotating the true 
> > > > > branch of another if (which doesn't have an else branch).
> > > > > 
> > > > > Mordante, in these docs, please document the "interesting" behavior 
> > > > > of the standard attribute on labels — annotating a label is different 
> > > > > from annotating the labeled statement itself. In particular,
> > > > > 
> > > > > [[likely]] case 1: case 2: foo(); break;
> > > > > 

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 14 inline comments as done.
Mordante added inline comments.



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

aaron.ballman wrote:
> Mordante wrote:
> > aaron.ballman wrote:
> > > Hmm, I'm on the fence about specifying `201803` for these attributes. 
> > > Given that this is only the start of supporting the attribute, do we want 
> > > to claim it already matches the standard's behavior? Or do we just want 
> > > to return `1` to signify that we understand this attribute but we don't 
> > > yet fully support it in common cases (such as on labels in switch 
> > > statements, etc)?
> > > 
> > > As another question, should we consider adding a C2x spelling 
> > > `[[clang::likely]]` and `[[clang::unlikely]]` to add this functionality 
> > > to C?
> > I was also somewhat on the fence, for now I'll change it to specify `1`.
> > 
> > I'll have a look at the C2x changes. Just curious do you know whether 
> > there's a proposal to add this to C2x?
> > I'll have a look at the C2x changes. Just curious do you know whether 
> > there's a proposal to add this to C2x?
> 
> There isn't one currently because there is no implementation experience with 
> the attributes in C. This is a way to get that implementation experience so 
> it's easier to propose the feature to WG14.
> I was also somewhat on the fence, for now I'll change it to specify `1`.

There seems to be an issue using the `1` so I used `2` instead. (Didn't want to 
look closely at the issue.)




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

Mordante wrote:
> aaron.ballman wrote:
> > Mordante wrote:
> > > aaron.ballman wrote:
> > > > Hmm, I'm on the fence about specifying `201803` for these attributes. 
> > > > Given that this is only the start of supporting the attribute, do we 
> > > > want to claim it already matches the standard's behavior? Or do we just 
> > > > want to return `1` to signify that we understand this attribute but we 
> > > > don't yet fully support it in common cases (such as on labels in switch 
> > > > statements, etc)?
> > > > 
> > > > As another question, should we consider adding a C2x spelling 
> > > > `[[clang::likely]]` and `[[clang::unlikely]]` to add this functionality 
> > > > to C?
> > > I was also somewhat on the fence, for now I'll change it to specify `1`.
> > > 
> > > I'll have a look at the C2x changes. Just curious do you know whether 
> > > there's a proposal to add this to C2x?
> > > I'll have a look at the C2x changes. Just curious do you know whether 
> > > there's a proposal to add this to C2x?
> > 
> > There isn't one currently because there is no implementation experience 
> > with the attributes in C. This is a way to get that implementation 
> > experience so it's easier to propose the feature to WG14.
> > I was also somewhat on the fence, for now I'll change it to specify `1`.
> 
> There seems to be an issue using the `1` so I used `2` instead. (Didn't want 
> to look closely at the issue.)
> 
> > I'll have a look at the C2x changes. Just curious do you know whether 
> > there's a proposal to add this to C2x?
> 
> There isn't one currently because there is no implementation experience with 
> the attributes in C. This is a way to get that implementation experience so 
> it's easier to propose the feature to WG14.

Nice to know. It seems the C2x wasn't at straightforward as I hoped, so I 
didn't implement it yet. I intend to look at it later. I first want the initial 
part done to see whether this is the right approach.



Comment at: clang/include/clang/Basic/AttrDocs.td:1697
+It's not allowed to annotate a statement with both ``likely`` and
+``unlikely``.  It's not recommended to annotate both branches of an ``if``
+statement with an attribute.

aaron.ballman wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > aaron.ballman wrote:
> > > > Why? I would expect this to be reasonable code:
> > > > ```
> > > > if (foo) [[likely]] {
> > > >   ...
> > > > } else if (bar) [[unlikely]] {
> > > >   ...
> > > > } else if (baz) [[likely]] {
> > > >   ...
> > > > } else {
> > > >   ...
> > > > }
> > > > ```
> > > > Especially because I would expect this to be reasonable code:
> > > > ```
> > > > switch (value) {
> > > > [[likely]] case 0: ... break;
> > > > [[unlikely]] case 1: ... break;
> > > > [[likely]] case 2: ... break;
> > > > [[unlikely]] default: ... break;
> > > > }
> > > > ```
> > > > As motivating examples, consider a code generator that knows whether a 
> > > > particular branch is likely or not and it writes out the attribute on 
> > > > all branches. Or, something like this:
> > > > ```
> > > > float rnd_value = 

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



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

Mordante wrote:
> aaron.ballman wrote:
> > Hmm, I'm on the fence about specifying `201803` for these attributes. Given 
> > that this is only the start of supporting the attribute, do we want to 
> > claim it already matches the standard's behavior? Or do we just want to 
> > return `1` to signify that we understand this attribute but we don't yet 
> > fully support it in common cases (such as on labels in switch statements, 
> > etc)?
> > 
> > As another question, should we consider adding a C2x spelling 
> > `[[clang::likely]]` and `[[clang::unlikely]]` to add this functionality to 
> > C?
> I was also somewhat on the fence, for now I'll change it to specify `1`.
> 
> I'll have a look at the C2x changes. Just curious do you know whether there's 
> a proposal to add this to C2x?
> I'll have a look at the C2x changes. Just curious do you know whether there's 
> a proposal to add this to C2x?

There isn't one currently because there is no implementation experience with 
the attributes in C. This is a way to get that implementation experience so 
it's easier to propose the feature to WG14.



Comment at: clang/include/clang/Basic/AttrDocs.td:1697
+It's not allowed to annotate a statement with both ``likely`` and
+``unlikely``.  It's not recommended to annotate both branches of an ``if``
+statement with an attribute.

Mordante wrote:
> Quuxplusone wrote:
> > aaron.ballman wrote:
> > > Why? I would expect this to be reasonable code:
> > > ```
> > > if (foo) [[likely]] {
> > >   ...
> > > } else if (bar) [[unlikely]] {
> > >   ...
> > > } else if (baz) [[likely]] {
> > >   ...
> > > } else {
> > >   ...
> > > }
> > > ```
> > > Especially because I would expect this to be reasonable code:
> > > ```
> > > switch (value) {
> > > [[likely]] case 0: ... break;
> > > [[unlikely]] case 1: ... break;
> > > [[likely]] case 2: ... break;
> > > [[unlikely]] default: ... break;
> > > }
> > > ```
> > > As motivating examples, consider a code generator that knows whether a 
> > > particular branch is likely or not and it writes out the attribute on all 
> > > branches. Or, something like this:
> > > ```
> > > float rnd_value = get_super_random_number_between_zero_and_one();
> > > if (rnd_value < .1) [[unlikely]] {
> > > } else if (rnd_value > .9) [[unlikely]] {
> > > } else [[likely]] {
> > > }
> > > ```
> > Right, annotating both/multiple branches of a control statement should be 
> > perfectly fine. Even `if (x) [[likely]] { } else [[likely]] { }` should be 
> > perfectly okay as far as the code generator is concerned (and we should 
> > have a test for that); it's silly, but there's no reason to warn against it 
> > in the compiler docs.
> > 
> > Aaron, notice that `if (x) [[likely]] { } else if (y) [[likely]] { }` is 
> > not actually annotating "both" branches of any single `if`-statement. There 
> > you're annotating the true branch of an if (without annotating the else 
> > branch), and then annotating the true branch of another if (which doesn't 
> > have an else branch).
> > 
> > Mordante, in these docs, please document the "interesting" behavior of the 
> > standard attribute on labels — annotating a label is different from 
> > annotating the labeled statement itself. In particular,
> > 
> > [[likely]] case 1: case 2: foo(); break;
> > case 3: [[likely]] case 4: foo(); break;
> > case 5: case 6: [[likely]] foo(); break;
> > 
> > indicates that the likely cases are 1, 4, 5, and 6. (1 and 4 because their 
> > labels are annotated; 5 and 6 because their control flow passes through an 
> > annotated statement. Case 3's control flow passes through an annotated 
> > label, but that doesn't matter to the standard attribute.)
> Aaron, the current `CodeGen` code for the `switch` statement allows all 
> branches to have a weight, for the `if` there are only two weights allowed. 
> As Arthur explained the chained `if`s are different statements, so this will 
> work properly.
> 
> Arthur, I agree we should add the documentation about the `switch`, but I'd 
> rather do that when the attributes are implemented for the `switch`. For now 
> I think it would make sense to add some documentation about the fact that the 
> attribute can be placed inside the `CompoundStmt`.
> Aaron, notice that if (x) [[likely]] { } else if (y) [[likely]] { } is not 
> actually annotating "both" branches of any single if-statement. There you're 
> annotating the true branch of an if (without annotating the else branch), and 
> then annotating the true branch of another if (which doesn't have an else 
> branch).

I agree, which is why I pointed it out -- I think users are going to be 
surprised by this behavior because it won't do what they expect it to do.

>  As Arthur 

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Mark de Wever via Phabricator via cfe-commits]

Mordante planned changes to this revision.
Mordante marked 14 inline comments as done.
Mordante added inline comments.



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

aaron.ballman wrote:
> Hmm, I'm on the fence about specifying `201803` for these attributes. Given 
> that this is only the start of supporting the attribute, do we want to claim 
> it already matches the standard's behavior? Or do we just want to return `1` 
> to signify that we understand this attribute but we don't yet fully support 
> it in common cases (such as on labels in switch statements, etc)?
> 
> As another question, should we consider adding a C2x spelling 
> `[[clang::likely]]` and `[[clang::unlikely]]` to add this functionality to C?
I was also somewhat on the fence, for now I'll change it to specify `1`.

I'll have a look at the C2x changes. Just curious do you know whether there's a 
proposal to add this to C2x?



Comment at: clang/include/clang/Basic/AttrDocs.td:1697
+It's not allowed to annotate a statement with both ``likely`` and
+``unlikely``.  It's not recommended to annotate both branches of an ``if``
+statement with an attribute.

Quuxplusone wrote:
> aaron.ballman wrote:
> > Why? I would expect this to be reasonable code:
> > ```
> > if (foo) [[likely]] {
> >   ...
> > } else if (bar) [[unlikely]] {
> >   ...
> > } else if (baz) [[likely]] {
> >   ...
> > } else {
> >   ...
> > }
> > ```
> > Especially because I would expect this to be reasonable code:
> > ```
> > switch (value) {
> > [[likely]] case 0: ... break;
> > [[unlikely]] case 1: ... break;
> > [[likely]] case 2: ... break;
> > [[unlikely]] default: ... break;
> > }
> > ```
> > As motivating examples, consider a code generator that knows whether a 
> > particular branch is likely or not and it writes out the attribute on all 
> > branches. Or, something like this:
> > ```
> > float rnd_value = get_super_random_number_between_zero_and_one();
> > if (rnd_value < .1) [[unlikely]] {
> > } else if (rnd_value > .9) [[unlikely]] {
> > } else [[likely]] {
> > }
> > ```
> Right, annotating both/multiple branches of a control statement should be 
> perfectly fine. Even `if (x) [[likely]] { } else [[likely]] { }` should be 
> perfectly okay as far as the code generator is concerned (and we should have 
> a test for that); it's silly, but there's no reason to warn against it in the 
> compiler docs.
> 
> Aaron, notice that `if (x) [[likely]] { } else if (y) [[likely]] { }` is not 
> actually annotating "both" branches of any single `if`-statement. There 
> you're annotating the true branch of an if (without annotating the else 
> branch), and then annotating the true branch of another if (which doesn't 
> have an else branch).
> 
> Mordante, in these docs, please document the "interesting" behavior of the 
> standard attribute on labels — annotating a label is different from 
> annotating the labeled statement itself. In particular,
> 
> [[likely]] case 1: case 2: foo(); break;
> case 3: [[likely]] case 4: foo(); break;
> case 5: case 6: [[likely]] foo(); break;
> 
> indicates that the likely cases are 1, 4, 5, and 6. (1 and 4 because their 
> labels are annotated; 5 and 6 because their control flow passes through an 
> annotated statement. Case 3's control flow passes through an annotated label, 
> but that doesn't matter to the standard attribute.)
Aaron, the current `CodeGen` code for the `switch` statement allows all 
branches to have a weight, for the `if` there are only two weights allowed. As 
Arthur explained the chained `if`s are different statements, so this will work 
properly.

Arthur, I agree we should add the documentation about the `switch`, but I'd 
rather do that when the attributes are implemented for the `switch`. For now I 
think it would make sense to add some documentation about the fact that the 
attribute can be placed inside the `CompoundStmt`.



Comment at: clang/include/clang/Basic/AttrDocs.td:1702
+
+At the moment the attribute is only implemented for an ``if`` statement.
+

aaron.ballman wrote:
> We should document whether we silently accept the attribute in other 
> locations and do nothing with it (I'm not keen on this approach because it 
> surprises users) or that we loudly diagnose the attribute in situations that 
> we don't support it in (I think this is a more user-friendly approach, but 
> may require a separate warning flag).
I'll update the documentation. I intend to continue to work on implementing 
these attributes so I feel adding a temporary diagnostic is not worth the 
effort.



Comment at: clang/include/clang/Basic/AttrDocs.td:1708
+
+  // compile with -Wimplicit-fallthrough
+  if (b) [[likely]] {

aaron.ballman wrote:
> Why does `-Wimplicit-fallthrough` matter?

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1697
+It's not allowed to annotate a statement with both ``likely`` and
+``unlikely``.  It's not recommended to annotate both branches of an ``if``
+statement with an attribute.

aaron.ballman wrote:
> Why? I would expect this to be reasonable code:
> ```
> if (foo) [[likely]] {
>   ...
> } else if (bar) [[unlikely]] {
>   ...
> } else if (baz) [[likely]] {
>   ...
> } else {
>   ...
> }
> ```
> Especially because I would expect this to be reasonable code:
> ```
> switch (value) {
> [[likely]] case 0: ... break;
> [[unlikely]] case 1: ... break;
> [[likely]] case 2: ... break;
> [[unlikely]] default: ... break;
> }
> ```
> As motivating examples, consider a code generator that knows whether a 
> particular branch is likely or not and it writes out the attribute on all 
> branches. Or, something like this:
> ```
> float rnd_value = get_super_random_number_between_zero_and_one();
> if (rnd_value < .1) [[unlikely]] {
> } else if (rnd_value > .9) [[unlikely]] {
> } else [[likely]] {
> }
> ```
Right, annotating both/multiple branches of a control statement should be 
perfectly fine. Even `if (x) [[likely]] { } else [[likely]] { }` should be 
perfectly okay as far as the code generator is concerned (and we should have a 
test for that); it's silly, but there's no reason to warn against it in the 
compiler docs.

Aaron, notice that `if (x) [[likely]] { } else if (y) [[likely]] { }` is not 
actually annotating "both" branches of any single `if`-statement. There you're 
annotating the true branch of an if (without annotating the else branch), and 
then annotating the true branch of another if (which doesn't have an else 
branch).

Mordante, in these docs, please document the "interesting" behavior of the 
standard attribute on labels — annotating a label is different from annotating 
the labeled statement itself. In particular,

[[likely]] case 1: case 2: foo(); break;
case 3: [[likely]] case 4: foo(); break;
case 5: case 6: [[likely]] foo(); break;

indicates that the likely cases are 1, 4, 5, and 6. (1 and 4 because their 
labels are annotated; 5 and 6 because their control flow passes through an 
annotated statement. Case 3's control flow passes through an annotated label, 
but that doesn't matter to the standard attribute.)



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2402
+def note_attribute_compatibility_here : Note<
+  "attribute '%0' declarered here">;
+def warn_attribute_likelihood_if_duplicated : Warning<

Typo: /declarered/declared/



Comment at: clang/lib/Sema/SemaStmt.cpp:585
+  // Warn when both the true and false branch of an if statement have the same
+  // likelihood attribute. It's not prohibited, but makes no sense.
+  const LikelyAttr *Likely = nullptr;

I agree with Aaron, this doesn't deserve a warning. Or at least there should be 
some thought put into the "threat model." (Are we protecting against someone 
typoing `[[likely]]`/`[[likely]]` when they meant `[[likely]]`/`[[unlikely]]`? 
But they weren't supposed to annotate both branches in the first place...)



Comment at: clang/lib/Sema/SemaStmtAttr.cpp:349
+<< Unlikely->getSpelling() << Unlikely->getRange();
+
+return;

Nit: Remove this blank line, for parallelism with lines 358-359.



Comment at: clang/test/SemaCXX/attr-likelihood.cpp:101
+}
+#endif

I'd like to see a case like `if (x) [[likely]] i=1;` just to prove that it 
works on statements that aren't blocks or empty statements. (My understanding 
is that this should already work with your current patch.)

I'd like to see a case like `if (x) { [[likely]] i=1; }` to prove that it works 
on arbitrary statements. This //should// have the same effect as `if (x) 
[[likely]] { i=1; }`. My understanding is that your current patch doesn't get 
us there //yet//. If it's unclear how we'd get there by proceeding along your 
current trajectory, then I would question whether we want to commit to this 
trajectory at all, yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85091

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for starting the implementation work on these attributes!




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

Hmm, I'm on the fence about specifying `201803` for these attributes. Given 
that this is only the start of supporting the attribute, do we want to claim it 
already matches the standard's behavior? Or do we just want to return `1` to 
signify that we understand this attribute but we don't yet fully support it in 
common cases (such as on labels in switch statements, etc)?

As another question, should we consider adding a C2x spelling 
`[[clang::likely]]` and `[[clang::unlikely]]` to add this functionality to C?



Comment at: clang/include/clang/Basic/AttrDocs.td:1697
+It's not allowed to annotate a statement with both ``likely`` and
+``unlikely``.  It's not recommended to annotate both branches of an ``if``
+statement with an attribute.

Why? I would expect this to be reasonable code:
```
if (foo) [[likely]] {
  ...
} else if (bar) [[unlikely]] {
  ...
} else if (baz) [[likely]] {
  ...
} else {
  ...
}
```
Especially because I would expect this to be reasonable code:
```
switch (value) {
[[likely]] case 0: ... break;
[[unlikely]] case 1: ... break;
[[likely]] case 2: ... break;
[[unlikely]] default: ... break;
}
```
As motivating examples, consider a code generator that knows whether a 
particular branch is likely or not and it writes out the attribute on all 
branches. Or, something like this:
```
float rnd_value = get_super_random_number_between_zero_and_one();
if (rnd_value < .1) [[unlikely]] {
} else if (rnd_value > .9) [[unlikely]] {
} else [[likely]] {
}
```



Comment at: clang/include/clang/Basic/AttrDocs.td:1702
+
+At the moment the attribute is only implemented for an ``if`` statement.
+

We should document whether we silently accept the attribute in other locations 
and do nothing with it (I'm not keen on this approach because it surprises 
users) or that we loudly diagnose the attribute in situations that we don't 
support it in (I think this is a more user-friendly approach, but may require a 
separate warning flag).



Comment at: clang/include/clang/Basic/AttrDocs.td:1708
+
+  // compile with -Wimplicit-fallthrough
+  if (b) [[likely]] {

Why does `-Wimplicit-fallthrough` matter?



Comment at: clang/include/clang/Basic/DiagnosticGroups.td:659
 def IgnoredAttributes : DiagGroup<"ignored-attributes">;
+def LikelihoodAttributeIf : DiagGroup<"likelihood-attribute-if">;
 def Attributes : DiagGroup<"attributes", [UnknownAttributes,

Is this because you expect people will want to control diagnostics on `if` 
statements separate from diagnostics in other syntactic locations?



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2399
 def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;
+def err_attribute_compatibility : Error<
+  "incompatible attributes '%0' and '%1'">;

This isn't needed, we already have `err_attributes_are_not_compatible`.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2401
+  "incompatible attributes '%0' and '%1'">;
+def note_attribute_compatibility_here : Note<
+  "attribute '%0' declarered here">;

We've already got `note_conflicting_attribute` for this as well.



Comment at: clang/lib/CodeGen/CGStmt.cpp:658
+  auto R = std::make_pair(false, false);
+  auto *AS = dyn_cast(Stmt);
+  if (!AS)

`const auto *`



Comment at: clang/lib/CodeGen/CGStmt.cpp:679-680
+
+  // The code doesn't protect against the same attribute on both branches.
+  // The frontend already issued a diagnostic.
+  std::pair LH = getLikelihood(Then);

I don't think the frontend should issue a diagnostic for that situation. I 
think codegen should handle chains appropriately instead as those seem 
plausible in reasonable code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85091

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

[Mark de Wever via Phabricator via cfe-commits]

Mordante created this revision.
Mordante added reviewers: rsmith, rjmccall.
Mordante added a project: clang.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a reviewer: aaron.ballman.
Herald added a project: LLVM.
Mordante requested review of this revision.

This contains the initial part of the implementation for the C++20 likelihood 
attributes.
For now it only handles them in an IfStmt, I want to add support for other 
statements after this one is done.

I was unsure whether it's preferred to have one patch for both the Sema and 
CodeGen part. If wanted I can split them easily.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85091

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticGroups.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/ParseDeclCXX.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGenCXX/attr-likelihood-if-branch-weights.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Preprocessor/has_attribute.cpp
  clang/test/SemaCXX/attr-likelihood.cpp
  clang/www/cxx_status.html
  llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp

Index: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
===
--- llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
+++ llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
@@ -24,7 +24,6 @@
 #include "llvm/IR/Metadata.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
-#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Utils/MisExpect.h"
@@ -48,10 +47,10 @@
 // 'select' instructions. It may be worthwhile to hoist these values to some
 // shared space, so they can be used directly by other passes.
 
-static cl::opt LikelyBranchWeight(
+cl::opt llvm::LikelyBranchWeight(
 "likely-branch-weight", cl::Hidden, cl::init(2000),
 cl::desc("Weight of the branch likely to be taken (default = 2000)"));
-static cl::opt UnlikelyBranchWeight(
+cl::opt llvm::UnlikelyBranchWeight(
 "unlikely-branch-weight", cl::Hidden, cl::init(1),
 cl::desc("Weight of the branch unlikely to be taken (default = 1)"));
 
Index: llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
===
--- llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
+++ llvm/include/llvm/Transforms/Scalar/LowerExpectIntrinsic.h
@@ -17,6 +17,7 @@
 
 #include "llvm/IR/Function.h"
 #include "llvm/IR/PassManager.h"
+#include "llvm/Support/CommandLine.h"
 
 namespace llvm {
 
@@ -31,6 +32,8 @@
   PreservedAnalyses run(Function , FunctionAnalysisManager &);
 };
 
+extern cl::opt LikelyBranchWeight;
+extern cl::opt UnlikelyBranchWeight;
 }
 
 #endif
Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -987,7 +987,7 @@
 
   [[likely]] and [[unlikely]] attributes
   https://wg21.link/p0479r5;>P0479R5
-  No
+  Clang 12 (partial)
 
 
   typename optional in more contexts
Index: clang/test/SemaCXX/attr-likelihood.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-likelihood.cpp
@@ -0,0 +1,101 @@
+// RUN: %clang_cc1 %s -verify -Wlikelihood-attribute-if
+// RUN: %clang_cc1 %s -verify -Wall
+// RUN: %clang_cc1 %s -DPEDANTIC -pedantic -verify -Wlikelihood-attribute-if
+
+#if PEDANTIC
+void g()
+{
+	if(true)
+		[[likely]] {}   // expected-warning {{use of the 'likely' attribute is a C++20 extension}}
+	else
+		[[unlikely]] {} // expected-warning {{use of the 'unlikely' attribute is a C++20 extension}}
+}
+#else
+void a()
+{
+	if(true)
+		[[likely]]   // expected-note {{attribute 'likely' declarered here}}
+		[[unlikely]] // expected-error {{incompatible attributes 'unlikely' and 'likely'}}
+		;
+}
+
+void b()
+{
+	if(true)
+		[[unlikely]] // expected-note {{attribute 'unlikely' declarered here}}
+		[[likely]]   // expected-error {{incompatible attributes 'likely' and 'unlikely'}}
+		;
+}
+
+void c()
+{
+	if(true) [[likely]] ;
+}
+
+void d()
+{
+	if(true) [[unlikely]] ;
+}
+
+void e()
+{
+	if(true) // expected-warning {{attribute 'likely' found in both true and false branch}}
+		[[likely]] {} // expected-note {{attribute 'likely' in true branch declared here}}
+	else
+		[[likely]] {} // expected-note {{attribute 'likely' in false branch declared here}}
+}
+
+void f()
+{
+	if(true) // expected-warning {{attribute 'unlikely' found in both true and false branch}}
+		[[unlikely]] {} // expected-note {{attribute 'unlikely' in true branch declared here}}
+	else
+