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