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

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

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

> 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 macro is example why I dislike the counting. If `MyAssert` has an `[[unlikely]]` attribute, then changing the number of `MyAssert` statements will influence the likelihood of the branches 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.)

I'm also not fond of GCC's implementation, but I think their implementation conforms with the standard. In hindsight I think it indeed makes more sense to only allow it on compound statements. That will require thinking about how to mark multiple cases as `[[likely]]` when falling through:

  switch(a) {
    case '0':
    case '1':
    case '2': [[likely]] { return 'a' - '0'; }      // All 3 cases likely?
  
    case '3':                                                      // neutral?
    case '4': [[likely]]()                                    // likely?
    case '5': [[unlikely]] {return 'a' - '0'; }  // unlikely?
  }

Of course this can be solved by only allowing it on the case labels:

  switch(a) {
   [[likely]] case '0':
   [[likely]] case '1':
   [[likely]] case '2':  { return 'a' - '0'; }    // All 3 cases likely
  
    case '3':                                                     // neutral
    [[likely]] case '4':                                    // likely
    [[unlikely case '5':  {return 'a' - '0'; }  // unlikely
  }

I fully agree the behaviour mandated by the standard is way too complex and user unfriendly. It would have been nice if there were simpler rules, making it easier to use and to teach. Still I think it would be best to use the complex approach now, since that's what the standard specifies. During that process we can see whether there are more pitfalls. Then we can discuss it with other vendors and see whether we can change the wording of the standard. Do you agree?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86559