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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from a documentation request, but you may want to wait a few days before committing in case Richard or John have opinions.

In D89210#2330466 <https://reviews.llvm.org/D89210#2330466>, @Mordante wrote:

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

That's right, thank you for the reminder!

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

Sounds good to me.

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

Okay, thank you for the explanation.



================
Comment at: clang/include/clang/Basic/AttrDocs.td:1815
 
+  switch(i) {
+    [[likely]] case 1:    // This value is likely
----------------
Can you add a second example that shows what to expect from fallthrough behavior? Perhaps something like:
```
switch (i) {
[[likely]] case 0: // This value and code path is likely
  ...
  [[fallthrough]];
case 1: // No likelihood attribute, code path is likely due to fallthrough
  break;
case 2: // No likelihood attribute, code path is neutral
  [[fallthrough]];
[[unlikely]] default: // This value and code path are both unlikely
  break;
}
```


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:375
+                                     ArrayRef<const Attr *> Attrs) {
+  // clang-format off
   switch (S->getStmtClass()) {
----------------
Mordante wrote:
> 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.
I tend to prefer whatever the default formatting is just so we don't have formatting comments all over the place (but I'm fine if the unique formatting is important for code readability).


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

https://reviews.llvm.org/D89210