[PATCH] D89210: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in SwitchStmt

[Aaron Ballman via Phabricator via cfe-commits]

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