[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

aaron.ballman added a comment. In D59467#1443173 , @Tyker wrote: > @lebedev.ri where are tests for AST serialization are located ? i didn't find > them. They live in clang\tests\PCH. In D59467#1440608 , @Tyker

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

Tyker added a comment. @lebedev.ri where are tests for AST serialization are located ? i didn't find them. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59467/new/ https://reviews.llvm.org/D59467 ___ cfe-commits mailing list

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

lebedev.ri added inline comments. Comment at: clang/include/clang/AST/Stmt.h:1795 + BranchHint getBranchHint() const { return IfStmtBits.Hint; } + `lib/AST/TextNodeDumper.cpp` will need to be taught to print it too (and astdumper test coverage to show that)

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

Tyker added a comment. @riccibruno Done CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59467/new/ https://reviews.llvm.org/D59467 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

Tyker updated this revision to Diff 192012. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59467/new/ https://reviews.llvm.org/D59467 Files: clang/include/clang/AST/Stmt.h clang/include/clang/Basic/Attr.td clang/include/clang/Basic/AttrDocs.td

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

riccibruno added a comment. Before any further review, could you please run `clang-format` on your patch (but not necessarily on the tests and not on the `*.td` files), wrap lines to 80 cols, and in general use complete sentences in comments (that is with proper punctuation and capitalization)

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

Tyker updated this revision to Diff 192004. Tyker added a comment. i implemented the semantic the changes for if for, while and do while statement and the AST change to if. can you review it and tell me if it is fine so i implement the rest. i didn't update the test so they will fail. CHANGES

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

riccibruno added a comment. To expand on the above, if you need to add a few bits to a statement/expression you can use the bit-field classes in `Stmt` (for example: `IfStmtBitfields`). You will also need to update the serialization code in `Serialization/ASTWriterStmt.cpp` and

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

aaron.ballman added a comment. In D59467#1439585 , @Tyker wrote: > about the semantic issue. > with the standard's latitude on where we can put the attribute, the > attribute can appear any where on the path of execution and must be matched > with the

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

Tyker added a comment. about the semantic issue. we will need for diagnostics purposes to detect attribute in branches during the semantic phase. so where and how can we store this information in the AST so that the CodeGen doesn't have to look through branches again for this attributes ?

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

riccibruno added a comment. In D59467#1439485 , @aaron.ballman wrote: > In D59467#1439473 , @riccibruno > wrote: > > > Hmm, it seems that an attribute is not allowed by the grammar in the > > `expression` or

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

aaron.ballman added a comment. In D59467#1439473 , @riccibruno wrote: > Hmm, it seems that an attribute is not allowed by the grammar in the > `expression` or `assignment-expression` of a `conditional-expression`. Was > that intended when `[[likely]]`

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

aaron.ballman added a reviewer: rsmith. aaron.ballman added a comment. Adding Richard for design strategy discussion. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8164 +def err_must_appear_after_branch : Error< + "%0 can only appear after a selection or

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

riccibruno added a comment. Hmm, it seems that an attribute is not allowed by the grammar in the `expression` or `assignment-expression` of a `conditional-expression`. Was that intended when `[[likely]]` was added ? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59467/new/

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

riccibruno added inline comments. Comment at: clang/lib/Sema/SemaStmtAttr.cpp:74 + ControlScope->getFlags() & Scope::FnTryCatchScope) + S.Diag(A.getLoc(), diag::err_must_appear_after_branch) << A.getName(); + } I think that you need to test for

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

riccibruno added a comment. Additionally I think that you need to: - add tests for templates - handle the ternary operator Also, as per the coding guidelines, you need to fix: - spelling of variables, eg `hint` -> `Hint`. - run `clang-format` on your patch. Comment at:

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

Tyker updated this revision to Diff 191862. Tyker added a comment. handled codegen for if, while, for and do/while, it will generate a @llvm.expect before the condition based on the attribute i changed slithly the semantic if (...) { //error [[likely]] ... } if (...) [[likely]] {

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

cpplearner added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8166 +def err_multiple_likelihood : Error< + "there can only be one %0 attribue in any attribute list">; +def err_mutuably_exclusive_likelihood : Error< attribue =>

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

Tyker updated this revision to Diff 191408. Tyker added a comment. Herald added a subscriber: jdoerfert. added diagnostics for contradictory attributes like for if: if (...) [[likely]] return 1; else [[likely]] return 2; handled the codegen for If to generate builtin_expect but i

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

Tyker updated this revision to Diff 191115. Tyker added a comment. i think we are suppose to hook likely/unlikely on builtin_expected, for if, for, while, switch. but i have no idea how we could hook it if we need to support catch. i added a revision with likely/unlikely and the correct

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

aaron.ballman added a comment. In D59467#1432675 , @Tyker wrote: > if likely/unlikely can be applied to any statement or label what should be > generated when it is applied to a statement that don't have any conditional > branch ? should it be ignored

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

lebedev.ri added a comment. In D59467#1432675 , @Tyker wrote: > if likely/unlikely can be applied to any statement or label what should be > generated when it is applied to a statement that don't have any conditional > branch ? should it be ignored

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

lebedev.ri added a comment. In D59467#1432675 , @Tyker wrote: > if likely/unlikely can be applied to any statement or label what should be > generated when it is applied to a statement that don't have any conditional > branch ? should it be ignored

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

Tyker added a comment. if likely/unlikely can be applied to any statement or label what should be generated when it is applied to a statement that don't have any conditional branch ? should it be ignored without warning or error ? CHANGES SINCE LAST ACTION

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

Tyker marked an inline comment as done. Tyker added a comment. I added the sema testes CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59467/new/ https://reviews.llvm.org/D59467 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1151 +def Likely : StmtAttr { + let Spellings = [CXX11<"", "likely", 201903>, CXX11<"clang", "likely">]; + let Documentation = [LikelyDocs]; I think this should have a C

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

aaron.ballman added a comment. I believe this has the incorrect modeling -- in the following code example, the attribute appertains to the substatement of the if statement, not the if statement itself: if (foo) [[likely]] { blah; } This falls out from:

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

Tyker updated this revision to Diff 191035. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59467/new/ https://reviews.llvm.org/D59467 Files: clang/include/clang/Basic/Attr.td clang/include/clang/Basic/AttrDocs.td clang/include/clang/Basic/DiagnosticSemaKinds.td

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

lebedev.ri added a comment. In D59467#1432587 , @Tyker wrote: > i added tests as you requested. i didn't add test for the codegen as it > wasn't affected. Ah right, there wasn't some other clang-specific spelling for this already it seems, so indeed

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

Tyker updated this revision to Diff 191034. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59467/new/ https://reviews.llvm.org/D59467 Files: clang/include/clang/Basic/Attr.td clang/include/clang/Basic/AttrDocs.td clang/include/clang/Basic/DiagnosticSemaKinds.td

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

Tyker updated this revision to Diff 191031. Tyker added a comment. i added tests as you requested. i didn't add test for the codegen as it wasn't affected. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59467/new/ https://reviews.llvm.org/D59467 Files:

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

lebedev.ri added a comment. Please always upload all patches with full context (`-U9`) Misses tests (sema, ast-dump, codegen) Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8164 +def err_likely_attr_invalid_placement : Error< + "likely annotation can't apear

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

Tyker created this revision. Tyker added a reviewer: aaron.ballman. Herald added a project: clang. Herald added a subscriber: cfe-commits. attributes after an if like: if (...) [[likely]] are now applied on the if instead of the following statement. i added the likely attribute in the