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

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 8 inline comments as done.
Mordante added a comment.

In D89210#2328378 <https://reviews.llvm.org/D89210#2328378>, @aaron.ballman wrote:

> 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:
>     ...

The likelihood is not affected by falling though, so all cases are distinct. This due to the standard's wording "A path of execution includes a label if and only if it contains a jump to that label." (http://eel.is/c++draft/dcl.attr.likelihood#2). The falling through would affect cases where the likelihood isn't on the labels, like:

  switch (something) {
   case 1: 
   case 2: [[likely]];
  }

I understood from our earlier conversations we didn't want to support that option, due to:

  switch (something) {
   case 1:
      if(foo()) break;
      // Falling through depends on the return value of foo()
      // is this still the path of execution of 'case 1'?
   case 2: [[likely]];
  }

Here we get to the cases where it might confuse the user what the affect of their attribute is. If we were to implement that not all cases are distinct. Implementing that would require flow analyzes.

I still have some more WIP patches regarding the likelihood. After they are landed I still intend to start a conversation with other compiler vendors to see whether we can agree on best practices regarding the likelihood attributes. Depending on the outcome of that conversation I might change the implementation in Clang.

> 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:
>     ...
>   }

In this case the user can use `[[unlikely]]`. If the user omits an attribute on `case 1` the label is considered neutral. Since `case 0` is positive, so it'll be the more likely path of execution.



================
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.
----------------
aaron.ballman wrote:
> 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.
> ```
adapted with a minor change "have neutral" -> "have `a` neutral". The other two have a similar change.


================
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
----------------
aaron.ballman wrote:
> Instead of `case`, how about we use `switch label`?
I liked your neutral, positive, negative wording so I rewrote this part using these words. I feel that looks better than the original wording.


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:375
+                                     ArrayRef<const Attr *> Attrs) {
+  // clang-format off
   switch (S->getStmtClass()) {
----------------
aaron.ballman wrote:
> 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?
It looks fine default formatted, I just thought we wanted to keep it compact. But I've no problem with keeping the code default formatted.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89210/new/

https://reviews.llvm.org/D89210