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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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

> In D86559#2236931 <https://reviews.llvm.org/D86559#2236931>, @Quuxplusone wrote:
>
>> It seems like this patch would basically "copy" the `[[likely]]` attribute from line C up to lines A and B, where it would reinforce the likelihood of path A and (maybe?) "cancel out" the unlikelihood of path B, without actually saying anything specifically about the likelihood of label C (which is surely what the programmer intended by applying the attribute, right?).
>
> Yes A is double `[[likely]]` and B is both `[[likely]]` and `[[unlikely]]`.  Based on http://eel.is/c++draft/dcl.attr.likelihood#:attribute,likely
> "The use of the likely attribute is intended to allow implementations to optimize for the case where paths of execution including it are arbitrarily more likely than any alternative path of execution that does not include such an attribute on a statement or label." 
> So it seem it's intended to see a goto as a part of a path of execution, since it's an unconditional jump.
>
> But I think the standard should be improved:
>
>   if(foo) {
>     [[likely]][[unlikely]] bar(1); // error: The attribute-token likely shall not appear in an
>     bar(2);                                      //  attribute-specifier-seq that contains the attribute-token unlikely.
>   }
>   if(foo) {
>     [[likely]] bar(1);         
>     [[unlikely]] bar(2);                // This contradiction in the `ThenStmt` is allowed
>   }
>   if(foo) {
>     [[likely]] bar(1);
>   } else {
>     [[likely]] bar(2);                   // This contradiction between the `ThenStmt` and `ElseStmt` is allowed
>   }
>
> So I'll work on a paper to discuss these issues. I already have some notes, but I would prefer to get more implementation experience before starting to write.
> IMO we should ignore conflicting likelihoods and issue a diagnostic. (For a switch multiple `[[likely]]` cases would be allowed, but still no mixing.)

I'd like to understand your reasoning for ignore + diagnose as opposed to count attrs (+ optionally diagnose) or other strategies. I can see pros and cons to basically anything we do here.

If we decide to ignore conflicting likelihoods, then I agree we should issue a diagnostic. However, I suspect that's going to come up frequently in practice because people are going to write macros that include these attributes. For instance, it's very sensible for a developer to put [[unlikely]] in front of an `assert` under the assumption this is telling the compiler "this assertion isn't likely to happen" when in fact it's saying "this branch containing the assertion is unlikely to be taken". This is one of the reasons why the GCC behavior of allowing these attributes on arbitrary statements feels like an awful trap for users to get into. If the attribute only applied to compound statements following a flow control construct, I think the average programmer will have a far better chance of not using this wrong. (I know that I said in the other thread we should match the GCC behavior, but I'm still uncomfortable with it because that behavior seems to be fraught with dangers for users, and this patch introduces new sharp edges, as @Quuxplusone pointed out.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86559