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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
