clang-misexpect: Profile Guided Validation of Performance Annotations in LLVM

[paulkirth]

Summary

This patch contains the basic functionality for reporting potentially incorrect usage of __builtin_expect() by comparing the developer's annotation against a collected PGO profile. A more detailed proposal and discussion appears on the CFE-dev mailing list (http://lists.llvm.org/pipermail/cfe-dev/2019-July/062971.html) and a prototype of the initial frontend changes appear here in D65300

We revised the work in D65300 by moving the misexpect check into the LLVM backend, and adding support for IR and sampling based profiles, in addition to frontend instrumentation.

We add new misexpect metadata tags to those instructions directly influenced by the llvm.expect intrinsic (branch, switch, and select) when lowering the intrinsics. The misexpect metadata contains information about the expected target of the intrinsic so that we can check against the correct PGO counter when emitting diagnostics, and the compiler's values for the LikelyBranchWeight and UnlikelyBranchWeight. We use these branch weight values to determine when to emit the diagnostic to the user.

A future patch should address the comment at the top of LowerExpectIntrisic.cpp to hoist the LikelyBranchWeight and UnlikelyBranchWeight values into a shared space that can be accessed outside of the LowerExpectIntrinsic pass. Once that is done, the misexpect metadata can be updated to be smaller.

In the long term, it is possible to reconstruct portions of the misexpect metadata from the existing profile data. However, we have avoided this to keep the code simple, and because some kind of metadata tag will be required to identify which branch/switch/select instructions are influenced by the use of llvm.expect

[nickdesaulniers]

Cool, I look forward to checking for outdated __builtin_expects in the Linux kernel, if they exist!

llvm/lib/Transforms/Utils/MisExpect.cpp

108

What's C, L, and U?

116

can std::plus be used here?

122

are the extra parens necessary?

142

Do you need to check first that NOps > 0 first? Otherwise you can likely move the creation of NOps below this conditional.

[paulkirth]

Address Code Review

• Give better names to extracted constants
• Remove extraneous call to getValue()
• Make integers const
• Remove extra braces
• Move NOps into branch
• Use std::plus

[paulkirth]

llvm/lib/Transforms/Utils/MisExpect.cpp

108

These are the constant integers for:
C = Index into the branch weight vector
L = likely branch weight
U = unlikely branch weight

The patch renames these to be a bit more transparent.

[nickdesaulniers]

llvm/lib/Transforms/Utils/MisExpect.cpp

108

const auto *IndexCInt

auto should be used when the type is used in the rhs of an assignment such as in a template specifier IMO. Ditto for below.

123

s/(uint64_t)0/0ULL/ might be simpler, but I don't know if long long == uint64_t on every host Clang can be built for...

[paulkirth]

Revert to use of auto when extracting metadata

[paulkirth]

llvm/lib/Transforms/Utils/MisExpect.cpp

123

I'm not sure if that is true, either.

I think its safe to follow the convention of existing LLVM code doing more or less the same thing, though, right?

[nickdesaulniers]

clang/lib/Frontend/CompilerInvocation.cpp

3451 ↗

(On Diff #217285)

The indentation here looks funny. Can you please run git-clang-format and commit the changes on top?

3456 ↗

(On Diff #217285)

LLVM style is no {} for single statement bodies.

llvm/include/llvm/IR/DiagnosticInfo.h

1009 ↗

(On Diff #217285)

Is this constructor used anywhere in this patch?

llvm/lib/IR/DiagnosticInfo.cpp

378 ↗

(On Diff #217285)

Why is this constructor defined out of line, when the other is in the header? Seems like they should either both be in the header, or both be out of line, with the goal being consistency.

[paulkirth]

Removes unused constructor, reformat code, and remove braces

[paulkirth]

llvm/include/llvm/IR/DiagnosticInfo.h

1009 ↗

(On Diff #217285)

No, sorry, I changed this interface and failed to remove the constructor.

llvm/lib/IR/DiagnosticInfo.cpp

378 ↗

(On Diff #217285)

I removed the other constructor, which was unused.

[nickdesaulniers]

llvm/lib/Transforms/Utils/MisExpect.cpp

150

Sorry if I'm going back and forth on this, but it may make sense to check the number of operands first, before accessing any.

159

do you need the misexpect:: qualifier here? I think your using statement above should make this one unnecessary?

[paulkirth]

Remove namespace qualifier & insert check for number of arguments

[paulkirth]

Change check for minimum number of metadata operands.

Branch weights must have at least 3 elements: metadata name, and a list of at least 2 branch weights

[paulkirth]

llvm/lib/Transforms/Utils/MisExpect.cpp

150

No problem. I've reintroduced the check. I was under the impression that metadata could not lack a 0th element, but even in that case I should have checked for additional elements.

There should always be at least 2 branch weights, otherwise branch weights are not necessary, and we should never end up here.

Should I add a comment saying that?

[nickdesaulniers]

llvm/lib/Transforms/Utils/MisExpect.cpp

150

Sorry, I meant "check the lower bound of the operands first, ..." as NOps might be 0 which would cause MD->getOperand(0) to fail, and RealWeights to be size -1. if (Nops < 1 || Nops > 3) return;

I think a comment would be helpful, too.

[paulkirth]

llvm/lib/Transforms/Utils/MisExpect.cpp

150

Thanks for the clarification and example. I think we're on the same page, but let me make sure, since I think I might have muddied the conversation by adding some incomplete context to my reasoning. Here is my reasoning in full, and a proposed comment. Let me know if there is something I'm missing, or that I've misunderstood.

I think that the check: if(NOps <3) return; is the correct semantics here.
Operand 0 will always exist if NOps >=3.
In the case of a switch instruction NOps > 3 needs to be supported.

Any profiling data with only less than 2 branch weights implies that the target is completely deterministic, and should not have branch weights associated with it. However, even if that were that happen, we should not emit any misexpect warnings,

Proposed comment:

// Only emit misexpect diagnostics if at least 2 branch weights are present. 
// Less than 2 branch weights means that the profiling metadata is incorrect, 
// is not branch weight metadata, or that the branch is completely deterministic. 
// In these cases we should not emit any diagnostic related to misexpect.

[nickdesaulniers]

llvm/lib/Transforms/Utils/MisExpect.cpp

150

Got it, yep that should be good.

[paulkirth]

Add comment to clarify choice of operands and reasoning about profiling metadata

[nickdesaulniers]

Looks good, will likely approve after these 2 questions.

llvm/lib/Transforms/Utils/MisExpect.cpp

157–158

would an assert be more appropriate then? If cases 1/2/3 above are all highly unlikely, I'd make this an assert rather than early return.

llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll

79 ↗

(On Diff #217528)

does the debug info need to be in all of these unit tests? If you still have the C sources handy, -g0 is extremely handy in generating more concise IR for unit tests.

[paulkirth]

llvm/lib/Transforms/Utils/MisExpect.cpp

157–158

I'm not sure an assert is appropriate. There are several types of profiling metadata, such as the value profiling metadata, and the types other than branch weight will have different numbers of arguments.

I'll look into the tags that fall under MD_profto see if any have less than 3 operands, but right now I'm not sure. I do know that other places that read the MD_prof tags use early return rather than an assert though.

The two examples I based my conventions on were:

• InstrProf.cpp:987 where VP metadata is extracted after first checking the number of operands.

• Metadata.cpp:1345 where branch weights are extracted using a less defensive approach, and do not check the number of operands before checking the metadata tag.

llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll

79 ↗

(On Diff #217528)

For this one, I think you're right, the debug info isn't needed, since we are checking that a diagnostic is never emitted.

In the other cases, I wanted to verify that the diagnostic flags the correct line number. If it is OK for the clang based tests to handle that aspect, then I can try to refactor the LLVM tests to be less precise about the output they are checking, and remove the debug data from the IR

[nickdesaulniers]

Ok, thanks for the iteration on code review.

[leonardchan]

clang-tools-extra/clang-misexpect/ClangMisExpect.cpp

64 ↗

(On Diff #215501)

Nit: The default and llvm_unreachable aren't needed here since you cover all cases already.

llvm/lib/IR/MDBuilder.cpp

315 ↗

(On Diff #217528)

nit: auto *IntType

llvm/lib/Transforms/Utils/MisExpect.cpp

55

nit: auto *B

124

nit: const unsigned NumUnlikelyTargets

129

nit: (double)ProfileCount / CaseTotal

[paulkirth]

@leonardchan can you revise your review against the latest revision? It looks to me as though you might have reviewed one of the first diffs instead of what is the latest code. For example the files in clang-tools have not been part of this revision for some time. Also when I try to respond to the comment, it appears against the very first diff. I'm happy to address any of the nits that remain, but want to make sure that what you reviewed and given approval for is what will be submitted.

[paulkirth]

Address nits

• Add pointers to uses of auto
• Remove redundant cast
• Use consistent type for integers

[paulkirth]

Actually address nits

[leonardchan]

Sorry about that. Still LGTM.

[lebedev.ri]

Trying to start reviewing this.
The llvm side of the patch is self contained; clang patch should be split into a dependent review.

clang/lib/CodeGen/CodeGenAction.cpp

626 ↗

(On Diff #218017)

This should be BadDebugInfoLoc (in getBestLocationFromDebugLoc() too)

clang/lib/CodeGen/CodeGenFunction.cpp

28–1361 ↗

(On Diff #218017)

Dubious changes, drop

clang/lib/Frontend/CompilerInvocation.cpp

3451–3454 ↗

(On Diff #218017)

Here you are checking that -Wmisexpect is passed to clang?
This doesn't seem idiomatic. Was this copied from somewhere?
Normally this is Diags.isIgnored(diag::warn_profile_data_misexpect, ???)

llvm/include/llvm/Transforms/Utils/MisExpect.h

1 ↗

(On Diff #218017)

__builtin_expect() is a clang-specific thing.
Can all instances of it in all comments be reworded to be more agnostic?

llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp

78–87 ↗

(On Diff #218017)

Why can't LLVMContext::MD_prof be reused?

82 ↗

(On Diff #218017)

Why do you need to move this line?

84 ↗

(On Diff #218017)

clang is not the only llvm frontend, this should not be so specific

llvm/lib/Transforms/Utils/MisExpect.cpp

30

This doesn't look correct.

33

<cstdint>

llvm/test/Transforms/LowerExpectIntrinsic/basic.ll

288–289 ↗

(On Diff #218017)

; CHECK: !1 = ??? ?

[paulkirth]

Thanks for the review.

I'm working on splitting up the patch now.

I should be able to address most of your comments, but I left some inline comments for clarification/discussion.

clang/lib/CodeGen/CodeGenAction.cpp

626 ↗

(On Diff #218017)

That should be in a separate patch, right?

llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp

78–87 ↗

(On Diff #218017)

It didn't seem appropriate to me, but that can be changed if it is the right thing to do.

However, I don't see code elsewhere that loops over MD_prof metadata checking its tag. I see lots of examples that check if the tag matches, and then exits if it doesn't match.

If I added "misexpect" as a new MD_proftag, then code in places like extractProfTotalWeight(...) in IR/Metadata.cpp would have to be updated. I'm not sure how pervasive that pattern is, but I did want to avoid introducing systemic changes like that.

https://github.com/llvm/llvm-project/blob/9976a5bc1db5a240378a61a68c0e182060bcb554/llvm/lib/IR/Metadata.cpp#L1336

82 ↗

(On Diff #218017)

I did this so that the condition is available to check when performing validation, and the metadata won't be replaced by the likely/unlikely values until after I've finished the validation. Having the condition available in when performing the check provides for better diagnostics output.

[paulkirth]

Address Review items

• minimize debug info in llvm tests
• sanitize paths in debug info
• use more complete checks in test for LowerExpectIntrisic
• remove references to __builtin_expect from backend code
  • update test code for new diagnostic messages
• rename checkClangInstrumentation to checkFrontendInstrumentation
• revert whitespace removal in CodeGenFunction.cpp
• use more idomatic check when adding diagnostic to the backend

[paulkirth]

In D66324#1652364, @lebedev.ri wrote:

Trying to start reviewing this.
The llvm side of the patch is self contained; clang patch should be split into a dependent review.

Looking at this, is it necessary to split up the patch? That will loose a lot of the previous comment/context, and I'd prefer to land this as a single change. Doing so has the benefit that if my patch needs to be reverted, it can be done all together. For example, if only the llvm patch is reverted, then it will likely cause other problems and make triage needlessly painful.

[paulkirth]

Remove extra include from CodeGenFunction.cpp

[paulkirth]

Fix comment for misexpect option

[phosek]

In D66324#1653198, @paulkirth wrote:

In D66324#1652364, @lebedev.ri wrote:

Trying to start reviewing this.
The llvm side of the patch is self contained; clang patch should be split into a dependent review.

Looking at this, is it necessary to split up the patch? That will loose a lot of the previous comment/context, and I'd prefer to land this as a single change. Doing so has the benefit that if my patch needs to be reverted, it can be done all together. For example, if only the llvm patch is reverted, then it will likely cause other problems and make triage needlessly painful.

I'd actually prefer to land this as a single change, it makes it easier to revert and look at the diff later.

[lebedev.ri]

Missing tests for __builtin_unpredictable().

clang/lib/CodeGen/CodeGenAction.cpp

626 ↗

(On Diff #218017)

Yeah, let's keep that for later.

369 ↗

(On Diff #218171)

Comment missing

clang/lib/Frontend/CompilerInvocation.cpp

3449 ↗

(On Diff #218171)

Err, this works? I'd expect ! to be missing here..

llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp

78–87 ↗

(On Diff #218017)

That's not exactly what i was asking.
I'm specifically talking about how "misexpect" and "branch_weigths" carry basically the same data,
are set basically in the same place always, with the difference that "misexpect" also has the switch case index.
This is both somewhat bloaty, and is prone to getting out of sync.

I have two questions:

1. Can "misexpect" metadata simply not be invented, but can existing LLVMContext::MD_misexpect simply be used?
2. Can "misexpect" be changed to not store the weights, but a reference to the LLVMContext::MD_misexpect metadata that will be emitted anyway?

llvm/test/Transforms/LowerExpectIntrinsic/basic.ll

141 ↗

(On Diff #218171)

Here and elsewhere:
is !prof !1 no longer present?

292–293 ↗

(On Diff #218171)

Should there be a FIXME, are some misexpect missing here?

[paulkirth]

llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp

78–87 ↗

(On Diff #218017)

Sorry for the misunderstanding.

I think what you are suggesting is to recreate the misexpect metadata from the MD_prof "branch_weights metadata. Is that right?

I think that may be possible, but it will add complexity. While the metadata approach is very straightforward. I think there is value in keeping this simple, and then iterating on it.

Also, LowerExpectIntrinsic.cpp has a comment near the top suggesting that the LikelyBranchWeight and UnlikelyBranchWeights be hoisted to a common space. This would significantly shrink the size of the misexpect metadata if it were accessible directly. However, that is a change that should happen separately from this patch, and might require further discussion to find the correct shared space.

Ultimately, however, I do not see a way to completely remove some kind of metadata tag, as we only need to perform misexpect checks when the intrinsic was used -- which should be a small minority of cases. If there is another way, then I am happy to hear it, but again, maybe that should be another patch.

[paulkirth]

clang/lib/Frontend/CompilerInvocation.cpp

3449 ↗

(On Diff #218171)

I ran check-clang and check-llvm. Are there more tests I should be running?

[paulkirth]

llvm/test/Transforms/LowerExpectIntrinsic/basic.ll

141 ↗

(On Diff #218171)

The misexpect metadata tags take those slots.

br i1 %tobool, label %if.then, label %if.end, !prof !0, !misexpect !1

Unless my understanding of how the tags work is flawed, I think this is the expected behavior

292–293 ↗

(On Diff #218171)

This is happenstance unfortunately.

; CHECK: !4 = !{!"branch_weights", i32 1, i32 2000, i32 1}
; CHECK: !5 = !{!"branch_weights", i32 2000, i32 1, i32 1}

In both of these cases the expected index is still 0 or 1, so the metadata tags are deduplicated.

If we want to check further, then we need to modify the test to select an index greater than 1.

[paulkirth]

Update LowerExpectIntrinsic/basic.ll to generate misexpect metadata for a switch

[paulkirth]

Add inline checks for misexpect tags to LowerExpectIntrinstic test

[paulkirth]

Add clang and LLVM tests for __builtin_unpredictable

[paulkirth]

Improve diagnostic output with profile counts

[lebedev.ri]

Okay, thank you for the patience, this looks good to me now too.
Please update patch description/subject - this patch is not a standalone tool.

clang/test/Profile/misexpect-branch-cold.c

4 ↗

(On Diff #219219)

Is there a test where -Wmisexpect isn't present, to verify that it is off-by-default?

llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp

78–87 ↗

(On Diff #218017)

nonnull

Yes.

...

Well, alright i guess, please add a note that this was at least considered into the patch description.

llvm/test/Transforms/LowerExpectIntrinsic/basic.ll

141 ↗

(On Diff #218171)

I was worried that here the !prof were no longer emitted, but only !misexpect were emitted.
This seems not to be the case, so all good i think.

[paulkirth]

clang/test/Profile/misexpect-branch-cold.c

4 ↗

(On Diff #219219)

We can add one, but is that necessary? Don't the tests for diagnostics cover those already?

[lebedev.ri]

clang/test/Profile/misexpect-branch-cold.c

4 ↗

(On Diff #219219)

To clarify: i'm interested in the case where the PGO data is provided but -Wmisexpect is *not* specified.

[paulkirth]

clang/test/Profile/misexpect-branch-cold.c

4 ↗

(On Diff #219219)

Actually, this is moot. That test already exists in misexpect-branch.c: line 6.

Since that file actually has a mismatched use of __builtin_expect(), it's a better candidate anyway.

// RUN: %clang_cc1 %s -O2 -o - -disable-llvm-passes -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify=foo

[paulkirth]

Add a new profdata file to use w/ misexpect-nonconst-switch.c

ASAN exposed an issue where a function may hash the same even if the number of counters is different. This means that when a PGO profile is read in, it is possible for an out of bounds write to occur due to a mismatch in the number of statements that require profile counters, and the number of counters initialized from the profile that was read in.

This change should allow the misexpect patch to reland w/o issue.

[chandlerc]

compiler-rt/trunk/lib/profile/xxhash.h

41–42 ↗

(On Diff #219645)

Sorry folks, but you can't do this.

You can't depend on ADT from compiler-rt currently.

There are at least two problems here:

First problem is that this pollutes the profile library with symbols from ADT. That really doesn't seem reasonable without *significant* and invasive changes to ADT. Otherwise building LLVM and linking it with the profile library will create an ODR violation (imagine different assert levels or different versions of LLVM buing built and the host toolchain).

Second, and much more critically, we haven't gotten to 100% relicensed on ADT, so it is critical that we not depend on it from runtime libraries.

Third, a lot of this code seems to use old license headers. Please do not add any code like this to LLVM, and instead use the new LLVM license for all new code.

For now, this patch (and any related patches) need to be reverted until these are addressed. Especially the license issues.

[gribozavr]

Reverted in r371598.

Another concern that I have is cross-compilation. LLVM's ADT is not set up to be cross-compiled like the rest of compiler-rt is.

[lebedev.ri]

In D66324#1665773, @gribozavr wrote:

Reverted in r371598.

Another concern that I have is cross-compilation. LLVM's ADT is not set up to be cross-compiled like the rest of compiler-rt is.

Uhm, i have a better question still - why xxhash is even there? it's not used in the diff, and was not reviewed, it just magically appeared in the last update of the patch:
https://reviews.llvm.org/D66324?vs=219617&id=219645

[paulkirth]

compiler-rt/trunk/lib/profile/xxhash.h

41–42 ↗

(On Diff #219645)

Sorry, this looks like a mismerge somehow. My patch should't have anything from compiler-rt. I think maybe a local change got rolled in when Petr landed my patch.

[phosek]

compiler-rt/trunk/lib/profile/xxhash.h

41–42 ↗

(On Diff #219645)

Sorry about that, this was my mistake, it seems two of my stales files got included in when I applied the patch locally. I'll revert the change and reland it correctly.

[paulkirth]

In D66324#1665784, @lebedev.ri wrote:

In D66324#1665773, @gribozavr wrote:

Reverted in r371598.

Another concern that I have is cross-compilation. LLVM's ADT is not set up to be cross-compiled like the rest of compiler-rt is.

Uhm, i have a better question still - why xxhash is even there? it's not used in the diff, and was not reviewed, it just magically appeared in the last update of the patch:
https://reviews.llvm.org/D66324?vs=219617&id=219645

It shouldn't have. My last change only modified a test file to not trigger a memory corruption bug. I'm filing a bug for that, but am waiting on a login for the bug tracker.

What I suspect happened is that when this landed some local changes got sucked in. I'm terribly sorry, it wasn't my intention to add anything significant to the patch.

[paulkirth]

Revert mismerge when landing.

[jrtc27]

llvm/trunk/include/llvm/IR/DiagnosticInfo.h

79 ↗

(On Diff #219733)

Is this not going to clash with the first caller to getNextAvailablePluginDiagnosticKind? DK_FirstPluginKind is special and shouldn't have anything after it.

[paulkirth]

llvm/trunk/include/llvm/IR/DiagnosticInfo.h

79 ↗

(On Diff #219733)

hmm, I wasn't aware of that. Is this documented somewhere? Also no tests seem to break from this, so that might be another area to address.

I'll upload a patch for this that moves the flag, and adds a note about the ordering.

[paulkirth]

llvm/trunk/include/llvm/IR/DiagnosticInfo.h

79 ↗

(On Diff #219733)

Please see D67648 for future reference of this defect.

[wenju]

We see error "clang (LLVM option parsing): for the --pgo-warn-misexpect option: may only occur zero or one times!" when clang::CompilerInvocation::CreateFromArgs and clang::ExecuteCompilerInvocation are called twice.

This line: if (Diags.isIgnored(diag::warn_profile_data_misexpect, SourceLocation()))
I think it should be !Diags.isIgnored.

[aykevl]

I have the same issue as @wenju. The second time I call ExecuteCompilerInvocation it will give the error above, leading me to believe some memory isn't properly cleared.

Note: the --pgo-warn-misexpect option is not passed in the compiler invocation.

[xbolva00]

Fix is easy, can you commit it as obvious?

[aykevl]

@xbolva00 I could, but to me this is not trivial (I'm not familiar with the code and I'm not sure what I would break). If it's easy for you, then please do (otherwise I can send a patch for review).
I have already worked around this issue in a different way (by running the compiler invocation in a separate process).

[jcelerier]

Also hit by this issue - a lot more people are certainly going to have it as this has started landing in Linux distros.

[AlexeySachkov]

Looks like the issue is fixed in https://github.com/llvm/llvm-project/commit/6d2b75e0887ee87e247756c4d51733616bb2f356

[rnk]

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

3457 ↗

(On Diff #219733)

Clang generally tries to avoid relying on LLVM option parsing if at all possible. It is not thread-safe, among other things. This causes essentially every compile to call llvm::cl::ParseCommandLineOptions, when previously it would only happen if the user passed unstable -mllvm flags.

[xbolva00]

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

3457 ↗

(On Diff #219733)

And it is buggy, if you add "!" as a fix, there are many test failures...
http://45.33.8.238/linux/18098/step_7.txt

[lebedev.ri]

@paulkirth @phosek @leonardchan @thakis @nickdesaulniers
It seems to me that this feature is still broken an unusable - https://bugs.llvm.org/show_bug.cgi?id=45073 -and the bug was reported more than half a year ago.
If no one is willing to step up and fix it, the i can only assume that no one is using this and or cares, and this broken incomplete feature should be reverted.

[xbolva00]

+1 for revert

[phosek]

I apologize for the late response, I somehow missed the earlier responses. We have successfully used this feature in Fuchsia and found it useful, but I agree that the issues raised need to be addressed. Unfortunately @paulkirth is no longer working on this project. I hope that someone from our team can take a look but it might take a few weeks. If you prefer, we could revert this change and then reland an improved version in the future?

[lebedev.ri]

In D66324#2336186, @phosek wrote:

I apologize for the late response, I somehow missed the earlier responses. We have successfully used this feature in Fuchsia and found it useful, but I agree that the issues raised need to be addressed. Unfortunately @paulkirth is no longer working on this project. I hope that someone from our team can take a look but it might take a few weeks. If you prefer, we could revert this change and then reland an improved version in the future?

I would very much prefer *NOT* not revert if someone is going to step up to work on these problems soon (within next 4 weeks?).

That being said, in light of that bug, my original doubts about the underlying data type (a novel MD_misexpect,
with structure different from MD_prof) have reappeared with double strength. I really think they should share underlying type.

[lebedev.ri]

In D66324#2336555, @lebedev.ri wrote:

In D66324#2336186, @phosek wrote:

I apologize for the late response, I somehow missed the earlier responses. We have successfully used this feature in Fuchsia and found it useful, but I agree that the issues raised need to be addressed. Unfortunately @paulkirth is no longer working on this project. I hope that someone from our team can take a look but it might take a few weeks. If you prefer, we could revert this change and then reland an improved version in the future?

I would very much prefer *NOT* not revert if someone is going to step up to work on these problems soon (within next 4 weeks?).

That being said, in light of that bug, my original doubts about the underlying data type (a novel MD_misexpect,
with structure different from MD_prof) have reappeared with double strength. I really think they should share underlying type.

I'll be posting a revert soon.

[haowei]

In D66324#2394687, @lebedev.ri wrote:

In D66324#2336555, @lebedev.ri wrote:

In D66324#2336186, @phosek wrote:

I apologize for the late response, I somehow missed the earlier responses. We have successfully used this feature in Fuchsia and found it useful, but I agree that the issues raised need to be addressed. Unfortunately @paulkirth is no longer working on this project. I hope that someone from our team can take a look but it might take a few weeks. If you prefer, we could revert this change and then reland an improved version in the future?

I would very much prefer *NOT* not revert if someone is going to step up to work on these problems soon (within next 4 weeks?).

That being said, in light of that bug, my original doubts about the underlying data type (a novel MD_misexpect,
with structure different from MD_prof) have reappeared with double strength. I really think they should share underlying type.

I'll be posting a revert soon.

Sorry for the late reply. Since @paulkirth is no longer working on this project. I can take over it and start working on it full time beginning next week. It would take me some time to get familiar with the code and work on the fix though. If you still prefer reverting this change, we can work on an improved version and reland it in the future.

[lebedev.ri]

In D66324#2395230, @haowei wrote:

In D66324#2394687, @lebedev.ri wrote:

In D66324#2336555, @lebedev.ri wrote:

In D66324#2336186, @phosek wrote:

I apologize for the late response, I somehow missed the earlier responses. We have successfully used this feature in Fuchsia and found it useful, but I agree that the issues raised need to be addressed. Unfortunately @paulkirth is no longer working on this project. I hope that someone from our team can take a look but it might take a few weeks. If you prefer, we could revert this change and then reland an improved version in the future?

I would very much prefer *NOT* not revert if someone is going to step up to work on these problems soon (within next 4 weeks?).

That being said, in light of that bug, my original doubts about the underlying data type (a novel MD_misexpect,
with structure different from MD_prof) have reappeared with double strength. I really think they should share underlying type.

I'll be posting a revert soon.

Sorry for the late reply. Since @paulkirth is no longer working on this project. I can take over it and start working on it full time beginning next week. It would take me some time to get familiar with the code and work on the fix though. If you still prefer reverting this change, we can work on an improved version and reland it in the future.

I think revert will result in an more easily reviewable fix, yes.

[lebedev.ri]

Reverted in rG6861d938e5c946cc7079d9849ef7560d07aa2d80, looking forward to the rework!

clang/include/clang/Basic/CodeGenOptions.def

 /// Choose profile instrumenation kind or no instrumentation.
 ENUM_CODEGENOPT(ProfileInstr, ProfileInstrKind, 2, ProfileNone)
 /// Choose profile kind for PGO use compilation.
 ENUM_CODEGENOPT(ProfileUse, ProfileInstrKind, 2, ProfileNone)
 CODEGENOPT(CoverageMapping , 1, 0) ///< Generate coverage mapping regions to
                                    ///< enable code coverage analysis.
 CODEGENOPT(DumpCoverageMapping , 1, 0) ///< Dump the generated coverage mapping
                                        ///< regions.
-CODEGENOPT(MisExpect , 1, 0) ///< Validate __builtin_expect with PGO counters
 
   /// If -fpcc-struct-return or -freg-struct-return is specified.
 ENUM_CODEGENOPT(StructReturnConvention, StructReturnConventionKind, 2, SRCK_Default)
 
 CODEGENOPT(RelaxAll          , 1, 0) ///< Relax all machine code instructions.
 CODEGENOPT(RelaxedAliasing   , 1, 0) ///< Set when -fno-strict-aliasing is enabled.
 CODEGENOPT(StructPathTBAA    , 1, 0) ///< Whether or not to use struct-path TBAA.
 CODEGENOPT(NewStructPathTBAA , 1, 0) ///< Whether or not to use enhanced struct-path TBAA.

clang/test/Profile/Inputs/misexpect-switch-default.proftext

+main
+# Func Hash:
+8712453512413296413
+# Num Counters:
+9
+# Counter Values:
+1
+20000
+20000
+4066
+11889
+0
+0
+4045
+0
+
+

clang/test/Profile/misexpect-branch.c

 // Test that misexpect detects mis-annotated branches
 
 // RUN: llvm-profdata merge %S/Inputs/misexpect-branch.proftext -o %t.profdata
-// RUN: %clang_cc1 %s -O2 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect
+// RUN: %clang_cc1 %s -O2 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect -debug-info-kind=line-tables-only
 
 #define likely(x) __builtin_expect(!!(x), 1)
 #define unlikely(x) __builtin_expect(!!(x), 0)
 
 int foo(int);
 int baz(int);
 int buzz();
 
 const int inner_loop = 100;
 const int outer_loop = 2000;
 
 int bar() {
   int rando = buzz();
   int x = 0;
-  if (likely(rando % (outer_loop * inner_loop) == 0)) { // expected-warning-re {{Potential performance regression from use of __builtin_expect(): Annotation was correct on {{.+}}% of profiled executions.}} expected-remark-re {{Potential performance regression from use of __builtin_expect(): Annotation was correct on {{.+}}% of profiled executions.}}
+  if (likely(rando % (outer_loop * inner_loop) == 0)) { // expected-warning-re {{Potential performance regression from use of __builtin_expect(): Annotation was correct on {{.+}}% of profiled executions.}}
     x = baz(rando);
   } else {
     x = foo(50);
   }
   return x;
 }
 

clang/test/Profile/misexpect-switch-default.c

 // Test that misexpect detects mis-annotated switch statements for default case
 
-// RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
-// RUN: %clang_cc1 %s -O2 -o - -disable-llvm-passes -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect
+// RUN: llvm-profdata merge %S/Inputs/misexpect-switch-default.proftext -o %t.profdata
+// RUN: %clang_cc1 %s -O2 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect -debug-info-kind=line-tables-only
 
 int sum(int *buff, int size);
 int random_sample(int *buff, int size);
 int rand();
 void init_arry();
 
 const int inner_loop = 1000;
 const int outer_loop = 20;
 const int arry_size = 25;
 
 int arry[arry_size] = {0};
 
 int main() {
   init_arry();
   int val = 0;
-
-  int j, k;
-  for (j = 0; j < outer_loop; ++j) {
-    for (k = 0; k < inner_loop; ++k) {
-      unsigned condition = rand() % 5;
-      switch (__builtin_expect(condition, 6)) { // expected-warning-re {{Potential performance regression from use of __builtin_expect(): Annotation was correct on {{.+}}% of profiled executions.}}
-      case 0:
-        val += sum(arry, arry_size);
-        break;
-      case 1:
-      case 2:
-      case 3:
-      case 4:
-        val += random_sample(arry, arry_size);
-        break;
-      default:
-        __builtin_unreachable();
-      } // end switch
-    }   // end inner_loop
-  }     // end outer_loop
+  int j;
+  for (j = 0; j < outer_loop * inner_loop; ++j) {
+      break;
+  }   // end outer_loop
 
   return 0;
 }

clang/test/Profile/misexpect-switch-only-default-case.c

 // Test that misexpect emits no warning when there is only one switch case
 
 // RUN: llvm-profdata merge %S/Inputs/misexpect-switch-default-only.proftext -o %t.profdata
-// RUN: %clang_cc1 %s -O2 -o - -disable-llvm-passes -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -fmisexpect
+// RUN: %clang_cc1 %s -O2 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect -debug-info-kind=line-tables-only
 
 // expected-no-diagnostics
 int sum(int *buff, int size);
 int random_sample(int *buff, int size);
 int rand();
 void init_arry();
 
 const int inner_loop = 1000;

clang/test/Profile/misexpect-switch.c

 // Test that misexpect detects mis-annotated switch statements
 
 // RUN: llvm-profdata merge %S/Inputs/misexpect-switch.proftext -o %t.profdata
-// RUN: %clang_cc1 %s -O2 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect
+// RUN: %clang_cc1 %s -O2 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect -debug-info-kind=line-tables-only
 
 int sum(int *buff, int size);
 int random_sample(int *buff, int size);
 int rand();
 void init_arry();
 
 const int inner_loop = 1000;
 const int outer_loop = 20;

llvm/lib/Transforms/Utils/MisExpect.cpp

 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/Support/BranchProbability.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/FormatVariadic.h"
 #include <bits/stdint-uintn.h>
 #include <numeric>
 
 #define DEBUG_TYPE "misexpect"
 
 using namespace llvm;
 using namespace misexpect;
 
 namespace {
 
 Instruction *getOprndOrInst(Instruction *I) {
   Instruction *Ret = nullptr;
   if (auto B = dyn_cast<BranchInst>(I)) {
     Ret = dyn_cast<Instruction>(B->getCondition());
   } else if (auto S = dyn_cast<SwitchInst>(I)) {
     Ret = dyn_cast<Instruction>(S->getCondition());
   }
   return Ret ? Ret : I;
 }
 
 void emitMisexpectDiagnostic(Instruction *I, LLVMContext &Ctx,
                              double PercentageCorrect) {
   auto PerString = formatv("{0:P}", PercentageCorrect);
   Twine Msg(PerString);
   Instruction *Cond = getOprndOrInst(I);
   Ctx.diagnose(DiagnosticInfoMisExpect(Cond, Msg));
 }
 
 } // namespace
 
 namespace llvm {
 namespace misexpect {
 
 void verifyMisExpect(Instruction *I, const SmallVector<uint32_t, 4> &Weights,
                      LLVMContext &Ctx) {
   if (auto *MisExpectData = I->getMetadata(LLVMContext::MD_misexpect)) {
if (MisExpectDataName &&
         emitMisexpectDiagnostic(I, Ctx, Percentage);
     }
   }
 }
 
 void checkClangInstrumentation(Instruction &I) {
   if (auto *MD = I.getMetadata(LLVMContext::MD_prof)) {
     unsigned NOps = MD->getNumOperands();
     // Operand 0 is a string tag "VP":
     if (MDString *Tag = cast<MDString>(MD->getOperand(0))) {
       if (NOps > 1 && Tag->getString().equals("branch_weights")) {
         SmallVector<uint32_t, 4> RealWeights(NOps - 1);
         for (unsigned i = 1; i < NOps; i++) {
           ConstantInt *Value =
               mdconst::dyn_extract<ConstantInt>(MD->getOperand(i));
           RealWeights[i - 1] = Value->getZExtValue();
         }
-        misexpect::verifyMisExpect(&I, RealWeights, I.getParent()->getParent()->getContext());
+        misexpect::verifyMisExpect(&I, RealWeights, I.getContext());
       }
     }
   }
 }
 
 } // namespace misexpect
 } // namespace llvm
 #undef DEBUG_TYPE