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