[PATCH] D86559: [Sema, CodeGen] Allow [[likely]] and [[unlikely]] on labels

[Staffan Tjernstrom via Phabricator via cfe-commits]

staffantj added a comment.

In D86559#2242586 <https://reviews.llvm.org/D86559#2242586>, @Mordante wrote:

>> That is exactly the behavior I am coming to believe we should follow. You can either write it on a compound statement that is guarded by a flow control decision (`if`/`else`/`while`) or you can write it on a label, otherwise the attribute is parsed and ignored (with a diagnostic). This feels like the right mixture of useful with understandable semantics so far, but perhaps we'll find other examples that change our minds.
>>
>> The fallthrough behavior question has one more case we may want to think about:
>>
>>   switch (x) {
>>   case 0:
>>   case 1:
>>   [[likely]] case 2: break;
>>   [[unlikely]] default:
>>   }
>>
>> Does this mark cases `0` and `1` as being likely or not? I could see users wanting to use shorthand rather than repeat themselves on all the cases. However, I'm also not certain whether there would be any performance impact if we marked only `case 2` as likely and left cases `0` and `1` with default likelihood. My gut feeling is that this should only mark `case 2`, but others may have different views.
>
> Not according to the standard: "A path of execution includes a label if and only if it contains a jump to that label."
> However for an if statement I use 2 weights: 2000 for likely, 1 for unlikely. (These can be configured by a command-line option already used for `__builtin_expect`.)
> For a switch I intent to use 3 weights: 2000 for likely, (2000 + 1)/2 for no likelihood, 1 for unlikely. `__builtin_expect` doesn't have a 'neutral' values since in a switch you can only mark one branch as likely. Instead of adding a new option I picked the midpoint.
> So the weights in your example would be:
>
>   > switch (x) {
>   case 0:  // 1000
>   case 1:  // 1000
>   [[likely]] case 2: break; // 2000
>   [[unlikely]] default: // 1
>    }
>
> In this case the user could also write:
>
>   > switch (x) {
>   case 0:  // 1000
>   case 1:  // 1000
>   case 2: break; // 1000
>   [[unlikely]] default: // 1
>    }
>
> where the values 0, 1, 2 still would be more likely than the default statement. Obviously this will be documented when I implement the switch.
> How do you feel about this approach?

As one of the SG14 industry members driving this, I'm firmly in the camp that this is what we're expecting. In the first case the 1/2 case are "neutral". This is a very explicit, and local, marker. Anything else makes it so vague as to be unusable for fine tuned code.

I should also make the point that we are not talking about a feature that is expected, or indeed should be, used by anyone other than someone with an exceedingly good understanding of what is going on. This is not a "teach everyone about it, it's safe" feature. It's there to produce a very fine-grained control in those cases where it really matters, and where profiling-guided optimizations would produce exactly the wrong result. Using this feature should be an automatic "is this needed" question in a code review. It is needed sometimes, just rarely.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86559