This is an archive of the discontinued LLVM Phabricator instance.

[Sema, CodeGen] Implement [[likely]] and [[unlikely]] in SwitchStmt
ClosedPublic

Authored by Mordante on Oct 11 2020, 11:12 AM.

Details

Summary

This implements the likelihood attribute for the switch statement. Based on the discussion in D85091 and D86559 it only handles the attribute when placed on the case labels or the default labels.

It also marks the likelihood attribute as feature complete. There are be more QoI patches in the pipeline.

Diff Detail

Event Timeline

Mordante requested review of this revision.Oct 11 2020, 11:12 AM
Mordante created this revision.

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:
  ...
}
clang/include/clang/Basic/AttrDocs.td
1727

multiple ''case'' labels or the ''default'' label

1729–1731

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

Instead of case, how about we use switch label?

1740

a case or default label of a switch statement

clang/lib/CodeGen/CGStmt.cpp
375

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?

1689

I'd personally skip the formatting markers and just let clang-format do its thing.

1715

Same suggestion here as above.

clang/test/CodeGenCXX/attr-likelihood-switch-branch-weights.cpp
162

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.

Mordante marked 8 inline comments as done.Oct 14 2020, 10:02 AM

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.

clang/include/clang/Basic/AttrDocs.td
1729–1731

adapted with a minor change "have neutral" -> "have a neutral". The other two have a similar change.

1732

I liked your neutral, positive, negative wording so I rewrote this part using these words. I feel that looks better than the original wording.

clang/lib/CodeGen/CGStmt.cpp
375

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.

Mordante updated this revision to Diff 298176.Oct 14 2020, 10:06 AM
Mordante marked 3 inline comments as done.

Address review comments:

  • Improved the documentation
  • Enabled clang-format on the new code
  • Added unit tests
  • Removed some unneeded code from the unit tests due to -disable-llvm-passes
  • Added information for the release notes
aaron.ballman accepted this revision.Oct 15 2020, 6:33 AM

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

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.

clang/include/clang/Basic/AttrDocs.td
1815

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;
}
clang/lib/CodeGen/CGStmt.cpp
375

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

This revision is now accepted and ready to land.Oct 15 2020, 6:33 AM
Mordante marked 2 inline comments as done.Oct 18 2020, 4:45 AM

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

Thanks for the review!

clang/include/clang/Basic/AttrDocs.td
1815

I've added this example with minor modifications. Note case 1 is neutral since falling through has no effect.

clang/lib/CodeGen/CGStmt.cpp
375

I'll keep that in mind.

This revision was automatically updated to reflect the committed changes.
Mordante marked 2 inline comments as done.