Addresses the review comments.
Adds an extra test case to test whether the proper overload is called. The proper overload is a bit of a surprise so when the expected behaviour changes the overload test can be adjusted.

Friendly ping.

Thanks for the review!

Improves the code based on @ldionne's suggestion

I'll rebase the patch and then have a look at your suggestion.

Thanks for accepting and committing the patch.

Thanks for the feedback!

Update the patch to match the behaviour where the attribute only affects the substatement of an if statement. This means the likelihood attributes on labels are accepted but are a no-op. Since they're a no-op the documentation doesn't mention them.

Thanks for your feedback during the review.

Addresses review comments, mainly:

• Improving the documentation
• Removing the AST bits since they're no longer required

In D85091#2250657, @rsmith wrote:

Looking specifically for attributes in the 'then' and 'else' cases of an if seems like a fine first pass at this, but I think this is the wrong way to lower these attributes in the longer term: we should have a uniform treatment of them that looks for the most recent prior branch and annotates it with weights. We could do that either by generating LLVM intrinsic calls that an LLVM pass lowers, or by having the frontend look backwards for the most recent branch and annotate it. I suppose it's instructive to consider a case such as:

void mainloop() noexcept; // probably terminates by calling `exit`
void f() {
  mainloop();
  [[unlikely]];
  somethingelse();
}

... where the [[unlikely]]; should probably cause us to split the somethingelse() out into a separate basic block that we mark as cold, but should not cause f() itself or its call to mainloop() to be considered unlikely -- except in the case where mainloop() can be proven to always terminate, in which case the implication is that it's unlikely that f() is invoked. Cases like this probably need the LLVM intrinsic approach to model them properly.

We indeed considered similar cases and wondered whether it was really intended to work this way. Since this approach seems a bit hard to explain to users, I changed the code back to only allow the attribute on the substatement of the if and else. For now I first want to implement the simple approach, which I expect will be the most common use case. Once finished we can see what the next steps will be.

In D86559#2250106, @staffantj wrote:

In D86559#2250034, @aaron.ballman wrote:

In D86559#2243575, @staffantj wrote:

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?

We did have discussions in SG14 about the possible need for "inverse PGO" optimization passes, but we didn't have the compiler dev expertise to know if we were on a good track or not. From the 8 years I've spent working on this kind of code, I've never mixed the two, since the interactions are just too unpredictable. What I will frequently do however, is compare benchmarks between PGO generated code, and our hand-specified optimizations. Sometimes the PGO detects interesting program flow that a mere human brain could not have envisioned, which we can then take advantage of (or de-prioritize) as needed.

Thanks again for your feedback. Once we agree on what to do with the Likelihood state in the output I'll finish the changes and submit a new patch.

Reworked the patch to match the behaviour as discussed in D86559.

• The likelihood attributes only have an effect when used directly on the ThenStmt and ElseStmt.
• Conflicting attributes on the ThenStmt and ElseStmt generate a diagnostic.
• Moved the likelihood determination from the CodeGen to the Sema. This requires the state to be stored in the AST.
• Updated the AST functions for the new likelihood bits.
• Updated the documentation.

In D86559#2243583, @staffantj wrote:

The use case for this becomes clearer when considering that the attribute that will be used 95% of the time is [[unlikely]]. You may have a constructor that you wish to ask the compiler to please, please, do not inline this, in a particular function, even if the rest of that function is either likely or neutral.

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

In D86559#2242713, @aaron.ballman wrote:

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

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.

I fixed the issues locally. Before I resubmit them I'd rather discuss in D86559 the direction we want to take, so we can avoid an additional review cycle.

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

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

Addresses review comments:

• Adds support for c2x using [[clang::likely]] and [[clang::unlikely]]
• Remove warnings about conflicting likelihoods, except those required by the Standard
• The attributes can now be placed inside the compound statement in of the ThenStmt and ElseStmt

Addresses review comments.

Addresses review comments.

I added void g() since that's valid code which also caused an assertion failure. So the issue isn't in the error recovery but in determining the required IntRange. It seems the code doesn't take http://eel.is/c++draft/expr.cond#2.1 into account.

Addresses the review comments.

Rebased the patch and updated the unit tests.

In D71142#1967409, @aaron.ballman wrote:

In D71142#1967315, @xbolva00 wrote:

Any comments?

@rsmith @aaron.ballman

There are outstanding review comments, so I'm waiting for the patch author to address those, unless I've missed a question somewhere?

Take all \params and function parameters in account when looking for a solution and find the best total solution.

Currently all unmapped parameters are taken into account.
I cannot imagine a use case where taking the mapped ones into account could influence the outcome.
Also I personally don't like breaking any existing correct documentation.
Do you have some example?

Sorry it took a while.

Implements the multiline using a derived class.

In D63059#1920509, @ldionne wrote:

In D63059#1920398, @Mordante wrote:

Thanks for the confirmation it's an ABI break. I like your suggestion to use a derived class, so I'll try that solution first.

I'm not 100% sure that solution addresses the ABI break problem, though. I'd have to think about it harder and run it through at least another person to be confident.

Friendly ping.

Thanks for the confirmation it's an ABI break. I like your suggestion to use a derived class, so I'll try that solution first.

Seems it requires CMake 3.8 to use CXX_STANDARD 17, so reverted the commit for now.

CXX_STANDARD_REQUIRED is already set by the main LLVM CMake script.

Good catch, I'll update the patch.

Addresses @EricWF's comment.

@mclow.lists, @ldionne is the change of the size of __l_anchor an ABI break?

Noticed I didn't stage my last hunk. Don't use BENCHMARK_DIALECT_FLAG which no longer is set.

It builds for me, but I had CMAKE_CXX_STANDARD set to 17 for this build. It seems the method used to use C++17 for the benchmarks didn't work. I feel fixing this issue should not be part of this commit. So I created D75955 to fix the issue. With that patch applied it also works for me when CMAKE_CXX_STANDARD set to 14. This is the default value.

Added unit tests as requested and rebased against master.

Thanks for looking into these patches!

Rebased against master.

Thanks for the patch! I've some minor nits, but other then that it looks fine.

@gribozavr2 what's your opinion of this patch?

Friendly ping.

Added some C++ tests for similar c-style casts.

In D74860#1883415, @rjmccall wrote:

Can we test the same thing in C++?

Thanks for your patch. I also would like to get better fixits for the arguments.

Rebased as requested.

Thanks for the review and committing! (BTW I have commit access.)

Friendly ping.

In D72231#1882658, @rjmccall wrote:

Okay. Can we raise this with the kernel folks instead of just assuming they'll be opposed? An obvious patch to fix a few dozen places where they're hit by a warning they intentionally enabled really doesn't seem like a burden. If they push back, fine, we can enable the warning without covering enums.

In D72231#1879347, @rjmccall wrote:

In D72231#1878528, @nathanchance wrote:

There appear to a be semantic difference between GCC and clang with the current version of this patch which results in a lot of additional warnings in the Linux kernel: https://godbolt.org/z/eHFJd8

Warning about casting to an enum seems clearly correct and in scope for this warning. Warning about casting to _Bool seems clearly incorrect and should not be warned about at all.