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

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.
Herald added a subscriber: danielkiss.

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

> 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."
>
> A switch statement contains a jump to all of its labels, though. So all of those labels are included in the path of execution, which is not likely what's intended by the standard.

In the example above if `x == 0` there will be a jump to `case 0` which then falls through to `case 1` and `case 2` so `case 0` doesn't jump to `case 2` and thus doesn't "execute" the label.

>> if(a) {
>>
>>   [[likely]] return; // Ignored, not on the first statement
>>
>> }
>
> Agreed.
>
>> if(a)                      // Attribute not allowed on a declaration,
>>
>>   [[likely]] int i = 5;  // but I can't think about a good use-case
>>                          // for this code.
>
> This is a fun example because I can think of a somewhat reasonable use-case but we can't support it without a compound statement. :-D The declaration could be an RAII object,
>
>   if (a)
>     [[likely]] SomeRAIIObj Obj(*a);
>
> However, you're right that we cannot write the attribute there because a declaration-statement cannot be attributed (http://eel.is/c++draft/stmt.stmt#stmt.pre-1), so the attribute instead appertains to the declaration and not the statement. So the user would have to write:
>
>   if (a) [[likely]] {
>     SomeRAIIObj Obj(*a);
>   }
>
> That said, I think this is enough of a corner case that requiring the compound statement is not really a burden. If we find users run into that often (they'd get an attribute ignored error if they tried), we could add a fix-it to help them out, but that doesn't seem necessary initially.

I had thought about RAII before and I think there it's also not a real issue. Your example does the same as:

  if (a)
     [[likely]] SomeRAIIObj{*a};
   

Here's  no declaration and the attribute is allowed. If the RAII object is used in a declaration I expect it usually will be inside a compound statement to create a scope where the object is alive. In that case the attribute is placed on the compound statement. I don't expect people to really write code like this, but it may happen when using macros.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86559