aaron.ballman added a comment.
Thank you for the continued work on this feature! I'd like to better understand
the behavior of fallthrough labels because I think there are two conceptual
models we could follow. When a label falls through to another label, should
both labels be treated as having the same likelihood or should they be treated
as distinct cases? e.g.,
switch (something) {
case 1:
...
[[likely]] case 2:
...
break;
case 3:
...
Should case 1 be considered likely because it falls through to case 2? From the
patch, it looks like your answer is "yes", they should both be likely, but
should that hold true even if case 1 does a lot of work and is not actually all
that likely? Or is the user expected to write `[[unlikely]] case 1:` in that
situation? We don't seem to have a test case for a situation where fallthrough
says something like that:
switch (something) {
[[likely]] case 0:
...
// Note the fallthrough
[[unlikely]] case 1:
...
}
================
Comment at: clang/include/clang/Basic/AttrDocs.td:1727
+In a ``switch`` statement it's allowed to annotate multiple ``case`` labels
+with the same likelihood attribute. This makes
----------------
multiple ''case'' labels or the ''default'' label
================
Comment at: clang/include/clang/Basic/AttrDocs.td:1729-1731
+* all values with the ``likely`` attribute equally likely,
+* all values with the ``unlikely`` attribute equally unlikely,
+* all values without an attribute equally likely.
----------------
How about:
```
This makes:
* all labels without an attribute have neutral likelihood,
* all labels marked [[likely]] have equally positive likelihood, and
* all labels marked [[unlikely]] have equally negative likelihood.
```
================
Comment at: clang/include/clang/Basic/AttrDocs.td:1732
+* all values without an attribute equally likely.
+When a ``case`` has no likelihood attribute it is more likely to be the path of
+execution than a ``case`` with the ``unlikely`` attribute and it is less likely
----------------
Instead of `case`, how about we use `switch label`?
================
Comment at: clang/include/clang/Basic/AttrDocs.td:1740
+In Clang, the attributes will be ignored if they're not placed on
+* the case labels of a switch statement,
+* or on the substatement of an ``if`` or ``else`` statement.
----------------
a case or default label of a switch statement
================
Comment at: clang/lib/CodeGen/CGStmt.cpp:375
+ ArrayRef<const Attr *> Attrs) {
+ // clang-format off
switch (S->getStmtClass()) {
----------------
How ugly is the default clang-format formatting for this? If it's not too bad,
perhaps we just re-format the switch statement as a whole?
================
Comment at: clang/lib/CodeGen/CGStmt.cpp:1671
+ switch (LH) {
+ // clang-format off
+ case Stmt::LH_Unlikely: ++NumUnlikely; break;
----------------
I'd personally skip the formatting markers and just let clang-format do its
thing.
================
Comment at: clang/lib/CodeGen/CGStmt.cpp:1697
+ switch (LH) {
+ // clang-format off
+ case Stmt::LH_Unlikely: Result.push_back(Unlikely); break;
----------------
Same suggestion here as above.
================
Comment at: clang/test/CodeGenCXX/attr-likelihood-switch-branch-weights.cpp:161
+ }
+}
+
----------------
I'd also like to see some tests where the `default` label is likely or unlikely
to ensure the weights are correct there as well.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89210/new/
https://reviews.llvm.org/D89210
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
