[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-1111
+  /// 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<Stmt::Likelihood, const Attr *>
+getLikelihood(const Stmt *Stmt) {
+  if (const auto *AS = dyn_cast<AttributedStmt>(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