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

[Richard Smith - zygoloid via Phabricator via llvm-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<Stmt::Likelihood, const Attr *>
+getLikelihood(const Stmt *Stmt) {
+  if (const auto *AS = dyn_cast<AttributedStmt>(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