Paths
| Differential D59467
[clang] Adding the Likelihood Attribute from C++2a AbandonedPublic Authored by Tyker on Mar 16 2019, 4:36 PM. Tokens "Love" token, awarded by hcorion.
Details
Summary 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 necessary places Attr.td, AttrDocs.td. i added a diagnostics kind for C++2a and for likely not used on an if statement.
Diff Detail Event TimelineComment Actions Please always upload all patches with full context (-U99999)
Comment Actions i added tests as you requested. i didn't add test for the codegen as it wasn't affected. Comment Actions
Ah right, there wasn't some other clang-specific spelling for this already it seems, so indeed codegen needs wiring up too. Sema tests still missing. (correct usage of attribute in various places with different -std=? params, incorrect usage of attribute in incorrect places, etc)
Comment Actions 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: http://eel.is/c++draft/stmt.stmt#1. You model this by applying the attribute to the if statement, which will have the wrong semantics when you hook up the hints to the backend.
Comment Actions 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 ? Comment Actions
Disregard previous [deleted] comment from me. Comment Actions
That's the semantics we need to figure out. The standard talks about paths of execution including the attribute (http://eel.is/c++draft/dcl.attr.likelihood#2.sentence-1), so I imagine this means we should be supporting code like: void func(bool some_condition) {
if (some_condition) {
[[likely]];
} else {
[[unlikely]];
}
}While that may seem a bit silly (because you can add the attribute to the compound-statement), there are situations where you cannot do that, but the feature still has use. Consider: void func() {
try {
some_operation();
} catch (some_exception &e) {
[[likely]];
recover_without_killing_everyone_on_the_elevator();
}
}In some safety-critical situations, you may not care how fast the normal operation works but still need the failure mode to be highly optimized to avoid loss of life. We have other fun questions to figure out as well, such as whether to diagnose this code and what the semantics should be: void func(bool foo) {
if (foo) {
[[likely]];
[[unlikely]];
}
}
void func2(bool foo) {
if (foo) {
[[likely]];
} else {
[[likely]];
}
}We don't have to solve it all up front, but having an idea as to what the backend is going to do with this information may help us design the appropriate frontend behavior where the standard gives us this much latitude. Do you know how this is expected to hook in to LLVM? Comment Actions i think we are suppose to hook likely/unlikely on @llvm.expect 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 semantic (i think). i also added the required checks. i didn't find a way to detect when we are in a catch block. but it should perhaps be allowded. gcc has implemented the likely/unlikely attribute. //gcc implementation
if (i) [[likely]] { //warning
return 0;
}
if (i) { [[likely]] // no warning
return 0;
}Comment Actions 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 probably didn't do it the canonical way. i am awaiting advice of how to do it right. cpplearner added inline comments. Comment Actions handled codegen for if, while, for and do/while, it will generate a @llvm.expect before the condition based on the attribute if (...) { //error
[[likely]] ...
}
if (...) [[likely]] { // ok
...
}and added tests for AST, Semantic and codegen Comment Actions Additionally I think that you need to:
Also, as per the coding guidelines, you need to fix:
Comment Actions 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 ? Comment Actions Adding Richard for design strategy discussion.
Comment Actions
Attributes do not appertain to expressions, so yes, that was intentional when we added likely. (You can have an attribute applied to an expression statement, but that's at a level that doesn't seem too useful for these attributes.) Comment Actions
Good to know, thanks ! Comment Actions about the semantic issue. Comment Actions
I feel like this could be tracked in the StmtBits as well as via an attributed stmt node. We need to be able to handle cases like: if (foo) [[likely]] {
stmts...
}
if (bar) {
stmts...
[[likely]];
stmts...
}
if (baz)
[[likely]] stmt;The attributed statement is required because we want ast pretty printing to also print out the [[likely]] attribute usage and AST matchers to appropriately find the attribute, etc, and we need to know which statement the attribute was attached to in order to get the positioning correct. However, for diagnostic purposes, we likely want to know information like "has this path already been attributed" and that can either be through some extra data on an AST node, or it could be an Analysis pass like we do for [[fallthrough]] support with a separate pass for CodeGen.
You can generally add information to AST nodes, but pay close attention to things like adding bits to a bit-field that suddenly cause the object size to increase and see if there are ways to avoid that if possible (and if not, it's still fine). It might make sense to work your way backwards from the backend to see what possible ways we can support this feature. __builtin_expect may make sense for if statements, but for case and default labels in a switch, we may want to play with the branch weights. For catch handlers, we may need to thread information through yet another way. Once we know what kinds of paths we can actually do something interesting with, we may spot a pattern of how to expose the information through the AST. Comment Actions 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 Serialization/ASTReaderStmt.cpp. Comment Actions 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. Comment Actions 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) ? To run clang-format on your patch you can use clang-format-diff. This is the command I use : git diff -U0 --no-color --no-prefix --cached | clang-format-diff-7 -style=LLVM -i -v, with --cached replaced appropriately.
Comment Actions
They live in clang\tests\PCH.
I think this may be moving closer to the correct direction now, but I'm still curious to hear if @rsmith has similar opinions.
Tyker retitled this revision from [clang] Adding the Likely Attribute from C++2a to AST to [clang] Adding the Likelihood Attribute from C++2a. Comment Actions@aaron.ballman fixed based on feedback. the modification in clang/lib/Analysis/CFG.cpp switch (...) {
[[likely]] case 1:
...
[[fallthrough]];
default:
...
}Comment Actions It seems that the tests are not present in this diff ? Also, again, could you please:
Tyker added inline comments.
Comment Actions @riccibruno i fixed based on feedback everything except the CFG edit as i still need to analyse the situation. added AST and CodeGen for For, While, Do and CXXFor.
Tyker added inline comments.
Tyker added inline comments.
Comment Actions The ASTImporter.cpp looks good to me. (Becasue the BranchHint is a simple an enum, so we don't need to specifically import that as we would in case of e.g. an Expr.) Comment Actions I closed it originally because it was inactive and i was working on other reviews. but i learned this is not how it is supposed to be done.
Revision Contents
Diff 193284 clang/include/clang/AST/Stmt.h
clang/include/clang/AST/StmtCXX.h
clang/include/clang/AST/TextNodeDumper.h
clang/include/clang/Basic/Attr.td
clang/include/clang/Basic/AttrDocs.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Scope.h
clang/include/clang/Sema/Sema.h
clang/lib/AST/ASTImporter.cpp
clang/lib/AST/Stmt.cpp
clang/lib/AST/StmtCXX.cpp
clang/lib/AST/TextNodeDumper.cpp
clang/lib/Analysis/CFG.cpp
clang/lib/CodeGen/CGStmt.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/CodeGen/CodeGenFunction.cpp
clang/lib/Parse/ParseStmt.cpp
clang/lib/Sema/Scope.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaStmt.cpp
clang/lib/Sema/SemaStmtAttr.cpp
clang/lib/Sema/TreeTransform.h
clang/lib/Serialization/ASTReaderStmt.cpp
clang/lib/Serialization/ASTWriterStmt.cpp
clang/test/CodeGenCXX/cxx2a-likelihood-attr.cpp
clang/test/PCH/cxx2a-likelihood-attr.cpp
clang/test/SemaCXX/cxx17-compat.cpp
clang/test/SemaCXX/cxx2a-likelihood-attr.cpp
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Can you make this a scoped enumeration to avoid injecting these names everywhere ? (+ add a comment describing what it is used for)