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

This revision was automatically updated to reflect the committed changes. Mordante marked an inline comment as done. Closed by commit rG08196e0b2e1f: Implements [[likely]] and [[unlikely]] in IfStmt. (authored by Mordante). Changed prior to commit:

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

Mordante marked 2 inline comments as done. Mordante added inline comments. Comment at: clang/include/clang/AST/Stmt.h:1180 + + /// \returns the likelihood of the branches of an if statement. + static Likelihood getLikelihood(const Stmt *Then, const Stmt *Else);

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

Quuxplusone added inline comments. Comment at: clang/include/clang/AST/Stmt.h:1180 + + /// \returns the likelihood of the branches of an if statement. + static Likelihood getLikelihood(const Stmt *Then, const Stmt *Else); Mordante wrote: > aaron.ballman wrote:

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

Mordante marked an inline comment as done. Mordante added a comment. Thanks for your feedback during the review. Comment at: clang/include/clang/AST/Stmt.h:1180 + + /// \returns the likelihood of the branches of an if statement. + static Likelihood getLikelihood(const Stmt

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

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a commenting nit. Thank you for the work on this! Comment at: clang/include/clang/AST/Stmt.h:1180 + + /// \returns the likelihood of the

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

Mordante updated this revision to Diff 289979. Mordante marked an inline comment as done. Mordante added a comment. Addresses review comments, mainly: - Improving the documentation - Removing the AST bits since they're no longer required CHANGES SINCE LAST ACTION

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

Mordante marked an inline comment as done. Mordante added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:578 +static std::pair +getLikelihood(const Stmt *Stmt) { + if (const auto *AS = dyn_cast(Stmt)) aaron.ballman wrote: > Mordante wrote: > > rsmith

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

aaron.ballman added a comment. In D85091#2252632 , @Mordante wrote: > In D85091#2250657 , @rsmith wrote: > >> Looking specifically for attributes in the 'then' and 'else' cases of an >> `if` seems like a fine

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

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:578 +static std::pair +getLikelihood(const Stmt *Stmt) { + if (const auto *AS = dyn_cast(Stmt)) Mordante wrote: > rsmith wrote: > > Mordante wrote: > > > aaron.ballman wrote: > > > >

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

Mordante marked 2 inline comments as done. Mordante added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:578 +static std::pair +getLikelihood(const Stmt *Stmt) { + if (const auto *AS = dyn_cast(Stmt)) rsmith wrote: > Mordante wrote: > > aaron.ballman

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

rsmith added inline comments. Comment at: clang/include/clang/AST/Stmt.h:1105- + /// The likelihood of a branch being taken. + enum Likelihood { +LH_None, ///< No attribute set. +LH_Likely, ///< Branch has the [[likely]] attribute. +LH_Unlikely, ///<

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

Mordante marked 8 inline comments as done. Mordante added a comment. In 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

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

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:578 +static std::pair +getLikelihood(const Stmt *Stmt) { + if (const auto *AS = dyn_cast(Stmt)) rsmith wrote: > Sema doesn't care about any of this; can you move this code to CodeGen

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

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

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

Mordante marked 8 inline comments as done. Mordante added a comment. Thanks again for your feedback. Once we agree on what to do with the Likelihood state in the output I'll finish the changes and submit a new patch. Comment at: clang/include/clang/Basic/AttrDocs.td:1697 +It

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

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:1693 +The attributes are used to aid the compiler to determine which branch is +likely or unlikely to be taken. This is done by marking the first statement in +the branch with one of the two

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

Mordante updated this revision to Diff 288781. Mordante marked an inline comment as done. Mordante added a comment. Reworked the patch to match the behaviour as discussed in D86559 . - The likelihood attributes only have an effect when used directly on the

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

Mordante marked 7 inline comments as done. Mordante added inline comments. Herald added a subscriber: danielkiss. Comment at: clang/include/clang/Basic/Attr.td:1288 +def Likely : StmtAttr { + let Spellings = [CXX11<"", "likely", 201803>]; + let Documentation =

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

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1288 +def Likely : StmtAttr { + let Spellings = [CXX11<"", "likely", 201803>]; + let Documentation = [LikelihoodDocs]; Mordante wrote: > aaron.ballman wrote: > > Mordante wrote:

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

Mordante planned changes to this revision. Mordante marked 15 inline comments as done. Mordante added a comment. I fixed the issues locally. Before I resubmit them I'd rather discuss in D86559 the direction we want to take, so we can avoid an additional review

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

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:1726 + } + }]; +} Mordante wrote: > aaron.ballman wrote: > > Something else we should document is what our behavior is when the > > attribute is not immediately inside of

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

Mordante marked 4 inline comments as done. Mordante added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:1726 + } + }]; +} aaron.ballman wrote: > Something else we should document is what our behavior is when the attribute > is not

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

aaron.ballman added a comment. In D85091#2232588 , @Mordante wrote: > The current implementation matches GCC's behaviour. This behaviour aligns > best with my interpretation of the standard and GCC seems to have the most > mature implementation. I would

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

Mordante updated this revision to Diff 287264. Mordante marked 2 inline comments as done. Mordante added a comment. Addresses review comments: - Adds support for c2x using `[[clang::likely]]` and `[[clang::unlikely]]` - Remove warnings about conflicting likelihoods, except those required by the

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

Mordante marked 8 inline comments as done. Mordante added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1288 +def Likely : StmtAttr { + let Spellings = [CXX11<"", "likely", 201803>]; + let Documentation = [LikelihoodDocs]; aaron.ballman wrote:

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

aaron.ballman added inline comments. Comment at: clang/test/SemaCXX/attr-likelihood.cpp:101 +} +#endif Quuxplusone wrote: > aaron.ballman wrote: > > Mordante wrote: > > > aaron.ballman wrote: > > > > Mordante wrote: > > > > > Quuxplusone wrote: > > > > > > I'd

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

Quuxplusone added inline comments. Comment at: clang/test/SemaCXX/attr-likelihood.cpp:101 +} +#endif aaron.ballman wrote: > Mordante wrote: > > aaron.ballman wrote: > > > Mordante wrote: > > > > Quuxplusone wrote: > > > > > I'd like to see a case like `if (x)

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

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1288 +def Likely : StmtAttr { + let Spellings = [CXX11<"", "likely", 201803>]; + let Documentation = [LikelihoodDocs]; Mordante wrote: > Mordante wrote: > > aaron.ballman wrote:

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

Mordante marked 14 inline comments as done. Mordante added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1288 +def Likely : StmtAttr { + let Spellings = [CXX11<"", "likely", 201803>]; + let Documentation = [LikelihoodDocs]; aaron.ballman

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

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1288 +def Likely : StmtAttr { + let Spellings = [CXX11<"", "likely", 201803>]; + let Documentation = [LikelihoodDocs]; Mordante wrote: > aaron.ballman wrote: > > Hmm, I'm on the

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

Mordante planned changes to this revision. Mordante marked 14 inline comments as done. Mordante added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1288 +def Likely : StmtAttr { + let Spellings = [CXX11<"", "likely", 201803>]; + let Documentation =

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

Quuxplusone added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:1697 +It's not allowed to annotate a statement with both ``likely`` and +``unlikely``. It's not recommended to annotate both branches of an ``if`` +statement with an attribute.

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

aaron.ballman added a comment. Thank you for starting the implementation work on these attributes! Comment at: clang/include/clang/Basic/Attr.td:1288 +def Likely : StmtAttr { + let Spellings = [CXX11<"", "likely", 201803>]; + let Documentation = [LikelihoodDocs];

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

Mordante created this revision. Mordante added reviewers: rsmith, rjmccall. Mordante added a project: clang. Herald added subscribers: llvm-commits, hiraditya. Herald added a reviewer: aaron.ballman. Herald added a project: LLVM. Mordante requested review of this revision. This contains the