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

Tue Sep 1 10:21:59 PDT 2020

aaron.ballman added a comment.

In D86559#2243575 <https://reviews.llvm.org/D86559#2243575>, @staffantj wrote:

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

Thank you for chiming in!

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

That doesn't mean we should design something that's really hard to use for average folks too.

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

+1 but I would point out that when PGO is enabled, this attribute is ignored, so it's an either/or feature. Either you get tuned optimizations, or you get to guess at them, but the current design doesn't let you mix and match. Do see that as being a major issue with the design?

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

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

My point was that the standard doesn't say the jump has to be executed, just that it has to exist. I think we both agree with how we'd like to interpret this bit, but if we're going to write a paper trying to improve the wording in the standard, I think this is a minor thing we could perhaps clean up.

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

This is a good point. I also agree that more RAII uses are going to use a compound statement than not. I think we're in agreement that this isn't a case we need to do anything special for.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86559