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

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86559