This is an archive of the discontinued LLVM Phabricator instance.

[misexpect] Re-implement MisExpect Diagnostics
ClosedPublic

Authored by paulkirth on Dec 16 2021, 2:01 PM.

Details

Summary

Reimplements MisExpect diagnostics from D66324 to reconstruct its
original checking methodology only using MD_prof branch_weights
metadata.

New checks rely on 2 invariants:

  1. For frontend instrumentation, MD_prof branch_weights will always be populated before llvm.expect intrinsics are lowered.
  1. for IR and sample profiling, llvm.expect intrinsics will always be lowered before branch_weights are populated from the IR profiles.

These invariants allow the checking to assume how the existing branch
weights are populated depending on the profiling method used, and emit
the correct diagnostics. If these invariants are ever invalidated, the
MisExpect related checks would need to be updated, potentially by
re-introducing MD_misexpect metadata, and ensuring it always will be
transformed the same way as branch_weights in other optimization passes.

Frontend based profiling is now enabled without using LLVM Args, by
introducing a new CodeGen option, and checking if the -Wmisexpect flag
has been passed on the command line.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
paulkirth updated this revision to Diff 394995.Dec 16 2021, 2:14 PM

[misexpect] Fix Typo

paulkirth updated this revision to Diff 394997.Dec 16 2021, 2:23 PM

Fix bad diff in arctool

paulkirth published this revision for review.Dec 16 2021, 2:38 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 16 2021, 2:38 PM
paulkirth edited the summary of this revision. (Show Details)Dec 16 2021, 3:51 PM
  • [misexpect] Add missing tests and modify diagnostic
paulkirth updated this revision to Diff 395517.Dec 20 2021, 1:18 PM
  • [misexpect] Add missing tests and modify diagnostic
  • [misexpect] Add missing tests and modify diagnostic
ormris removed a subscriber: ormris.Jan 18 2022, 9:40 AM
tejohnson added inline comments.
clang/include/clang/Basic/CodeGenOptions.def
178

"Set when ..."

clang/lib/CodeGen/CodeGenAction.cpp
1209

Out of curiosity, since I am less familiar with clang than llvm, when is this path taken vs where this is done at line 336 in HandleTranslationUnit?

Also, it might be nice to have this code placed either consistently before or after the OptRecordFile handling in these two locations, so it is easier to compare.

llvm/docs/MisExpect.rst
3

I don't see a mention of -Wmisexpect in the document. Should there be user-facing clang documentation, this seems more specific to the LLVM implementation?

23

Suggest using "profile weight" not profiling counter since we don't really have profile counters associated with instructions (rather with edges in the MST, at least in the IR PGO).

Also, doesn't the profile weight being too low only handle the LIKELY case? What about something being marked UNLIKELY with a hot/high profile weight? edit: After looking through the implementation, I think the comparison only being done in this direction is based on the way it is implemented, but it is unclear from this documentation here how the comparison is handling things being off in either direction.

28

grammatical issue "prior to when ... being assigned" - suggest "are assigned".

40

s/count/weight/

Also, similar to my earlier comment, what about an expect UNLIKELY branch with a high profile weight?

49

As noted in my comment at the top, it would be good to have user-facing clang documentation, in which case the clang version of this option is -Rpass=misexpect.

55

Looking at the code, the -pgo-warn-misexpect seems to be useful for internal LLVM testing via 'opt', to mimic -Wmisexpect, so it probably should be documented as such.

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

Update function name.

llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
110

Is there a reason why this line has changed ordering w.r.t. setMetadata?

llvm/lib/Transforms/Utils/MisExpect.cpp
158

This is the only use of Index - can you just use MaxId directly? I guess the assumption is that the ExpectedWeights array has the same ordering as the RealWeights array, so we can check if the corresponding real weight from profile is colder than the most likely weight from the expect. Some comments to describe how the comparison is being done in this code more intuitively would help make it easier to understand.

167

Why is this called a "Threshold" and not just a "Probability"?

171

Is this too strict - i.e. what if they are off by only a small amount?

199

A lot of the code in this and the backend case above are similar - could you refactor the common handling into a helper?

Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2022, 5:15 PM
paulkirth updated this revision to Diff 414230.Mar 9 2022, 3:43 PM

Consolodate common code, clarify documentation, and address comments

paulkirth marked 6 inline comments as done.Mar 9 2022, 4:08 PM
paulkirth added inline comments.
clang/lib/CodeGen/CodeGenAction.cpp
1209

That was a good catch, I had forgotten to remove this after patching in line 366. I moved that a bit a few lines lower to fit more nicely, and removed this one entirely.

llvm/docs/MisExpect.rst
3

I will update the patch to include similar documentation this for clang.

Additionally, my understanding is that documentation for warnings is auto-generated from the tablegen, so that at least will be available automatically.

23

Maybe my mental model is off here, but doesn't the llvm.expect intrinsic mark a specific value as the 'likely' or expected value? So if you want to mark a branch as unlikely, you're essentially marking the other half of it as 'hot'.

We could change the comparison to compare all parts of the branch weights too, but the case of an 'unlikely' branch being too hot in my model is captured by the 'likely' branch becoming too 'cold'.

If that is incorrect, I'd really appreciate some guidance on how to model this more accurately.

40

I think I've reworded this to clarify things a bit more, but let me know if it still needs some polish.

llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
110

I think I actually restored this to what it was before the original MisExpect patch, but there is no need for that, so I've removed it.

llvm/lib/Transforms/Utils/MisExpect.cpp
171

Maybe. I don't know what a good default fudge factor would be, though. Maybe use 5%, and expose a flag that can override it?

Another approach is that the values used by llvm.expect can be changed via clang flags, which will update the weights checked against.

MisExpect was originally intended to be quite strict, so that developers would audit their code and re-evaluate the correctness of their annotations,or if they were needed at all.

I think I'm still of a mind that getting flagged by MisExpect indicates that a different annotation would be more beneficial, such as llvm.expect.with.probability.

Regardless, I agree its good to give users an option to relax the checking when they want to, so I will add an option that allows them to specify a scaling factor for the threshold.

paulkirth updated this revision to Diff 414523.Mar 10 2022, 4:05 PM

Add clang and llvm options to realx MisExpect checking.

The options take an integer, expected to bin in the range [0,100)
and reduce the threshold value we compare against by that percentage.

I modeled this on remark's hotness threshold option, which is close to what
we want here, but we would like this to use a percentage instead of the
profiling count directly.

tejohnson added inline comments.Mar 15 2022, 11:14 PM
clang/include/clang/Basic/CodeGenOptions.h
425

s/Tollerance/Tolerance/ (and elsewhere throughout patch)

clang/lib/Frontend/CompilerInvocation.cpp
1983

Add tests for expected errors?

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

Can you add a case where an unlikely() is wrong?

llvm/docs/MisExpect.rst
3

Should the clang documentation already be added to this patch? I couldn't find it.

23

Yes I agree that is how llvm.expect is implemented, and I can see from your implementation that it should be handling that correctly. It was just unclear from the way it is written here what was happening. I like the new wording which is less specific to the way the checking is implemented.

40

I think the new version is good, thanks.

llvm/include/llvm/Transforms/Utils/MisExpectTolleranceParser.h
12 ↗(On Diff #414523)

Is the end of the sentence incomplete? I.e. should be "both int and (something else)"?

26 ↗(On Diff #414523)

Fix line wrapping. But sentence could be simplified too - is this just saying that the input must be an integer?

42 ↗(On Diff #414523)

Where is this class used?

51 ↗(On Diff #414523)

I don't see where 'auto' is handled?

llvm/lib/Transforms/IPO/SampleProfile.cpp
1737

Document const parameter (e.g. "/*IsFrontend=*/false"), here and in other calls.

llvm/lib/Transforms/Utils/MisExpect.cpp
79

Suggest renaming to something more intuitive. E.g. getInstCondition?

181

typo in calculate

182

typo in collected

189

afaict this handling duplicates what is done in parseTolleranceOption when the tolerance comes in from the clang option. Can you just do this handling once, here?

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

Add a one sentence description at the top of the llvm tests about what they are testing.

4

The old PM is gone in the optimization pipeline, so these old PM lines in this and other tests and the comment about the new PM can go away.

paulkirth updated this revision to Diff 415990.Mar 16 2022, 2:44 PM

Address comments.

Added clang documentation.

Fixed various typos.

Updated tests per comments for clang and LLVM.

Removed dead class definition.

Removed clamping from tolerance parser.

paulkirth marked 10 inline comments as done.Mar 16 2022, 2:55 PM
paulkirth added inline comments.
clang/test/Profile/misexpect-branch.c
26

I've added one, but I'm not sure it tests anything meaningfully, since an unlikely path being too hot is really the same check as a likely path being too cold.

llvm/docs/MisExpect.rst
3

In the last update the clang documentation was a bit too close to the llvm documentation. I think they are distinct enough now to be separately useful, but maybe it's better to merge the two?

55

Yes, its useful to test llvm, but shouldn't opt be usable in the same way? To me it seems useful for opt to support the warning directly too, but I'm happy to defer here if you think that's confusing or shouldn't be the case.

llvm/lib/Transforms/Utils/MisExpect.cpp
79

That's definitely a better name. Thanks for the suggestion.

189

This is now the only place we check/modify the value.

tejohnson accepted this revision.Mar 17 2022, 3:16 PM

lgtm

Just some minor cleanup before submitting as noted below.

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

Comment is a little unclear, as it is testing both likely and unlikely branches.

10

nit: stray empty comment line?

26

Right, it is just for completeness to test / illustrate that the handling works in both directions.

llvm/docs/MisExpect.rst
3

Yeah there is a fair amount of overlap, but I at least would want the clang documentation you added. Seems fine to have the llvm internals documentation too, though.

55

Oh I'm not disagreeing with having the internal option and using it for opt, that's very useful. My comment was in the context of not having the user-facing clang documentation with the clang driver level option (which you since added). I was just suggesting you add a note that this option is an alternative to the clang option for use when e.g. testing via opt. Since there is now separate clang documentation I think it is less important now.

llvm/include/llvm/Transforms/Utils/MisExpectToleranceParser.h
27 ↗(On Diff #415990)

Given that there is only one callsite to this helper, I think just move it into the calling source file and avoid adding a new header.

llvm/lib/IR/LLVMContextImpl.h
1384

s/tollerate/tolerate/

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

I see you removed the NewPM-style invocations and kept the legacy ones - afaict opt will now convert the legacy style pass invocations to the new PM, but I think it is preferable to use the New PM style invocations.

This revision is now accepted and ready to land.Mar 17 2022, 3:16 PM

Add Clang release note as well please.

clang/docs/MisExpect.rst
59

value

paulkirth updated this revision to Diff 416356.Mar 17 2022, 4:34 PM
paulkirth marked 4 inline comments as done.

Fixup remaining outstanding issues

  • fix typos
  • add misexpect to clang release notes
  • restore new pm style opt invocations in llvm tests
  • move single header function into implementation file
  • clarify comment in test file
paulkirth marked 5 inline comments as done.Mar 17 2022, 4:39 PM
paulkirth added inline comments.
llvm/docs/MisExpect.rst
55

Thanks for the clarification.

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

yes, I misread your earlier comment. I have restored the New PM style opt invocations, and removed the legacy PM ones.

This revision was landed with ongoing or failed builds.Mar 17 2022, 4:48 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Mar 17 2022, 5:20 PM

Looks like this breaks tests: http://45.33.8.238/linux/71396/step_7.txt

Please take a look, and revert for now if it takes a while to fix.

These pass for me locally, so I'm reverting for now and will dig into this tomorrow.

fhahn added a subscriber: fhahn.Mar 18 2022, 3:16 AM

The test also failed in the Phabricator pre-commit CI, please keep an eye on it before re-submitting (failure link for latest diff was https://buildkite.com/llvm-project/premerge-checks/builds/83983#39c06525-7452-412d-af83-ae2cc2d30cdc)

paulkirth reopened this revision.Mar 24 2022, 12:46 PM
This revision is now accepted and ready to land.Mar 24 2022, 12:46 PM

Add missing GenerateArgs call to propagate flags to the backend outside of cc1

paulkirth added inline comments.Mar 24 2022, 12:56 PM
clang/lib/Frontend/CompilerInvocation.cpp
1554

I really don't understand why this step is necessary, or why in my local builds omitting the call to GenerateArgs works at all. I only arrived at this change by noticing other options do the same, and this seemed to fix the issue with tests using the repro instructions from the precommit bots.

It seems a bit strange that Clang parses the option, stores it to a CodeGenOption then puts it back as a string argument to be parsed again later and put into the same data structure. Is this a result of an earlier architecture in Clang? if so, should we reconsider its design?

Fix check for tolerance diagnostic since we now use GenerateArgs the same way as hotness threshold.

Since it has a default value, we need to also ensure that it uses a pattern similar to other options.

tejohnson added inline comments.Mar 28 2022, 9:57 AM
clang/lib/Frontend/CompilerInvocation.cpp
1554

My understanding is that this is required as part of the cc1 command line parsing. Not sure why it worked in your local builds but not elsewhere. There is some info here: https://clang.llvm.org/docs/InternalsManual.html#adding-new-command-line-option

@tejohnson thanks for pointing me to the document. I knew it had something to do w/ CC1 but missed that this was well documented.

Is there anything else that needs to be done, or do you think this is good to land again?

@tejohnson thanks for pointing me to the document. I knew it had something to do w/ CC1 but missed that this was well documented.

I was equally unaware of the documentation of this until you asked and I tried to come up with a reasonable explanation for you =)

Is there anything else that needs to be done, or do you think this is good to land again?

lgtm

This revision was automatically updated to reflect the committed changes.
tonic added a subscriber: tonic.Mar 28 2022, 10:52 PM

This is breaking the docs build with the following warning: "MisExpect.rst:document isn't included in any toctree." By default, docs warnings are treated as errors. Can you fix this?

reverted. I will fix this tomorrow. Sorry for the trouble

paulkirth reopened this revision.Mar 28 2022, 11:23 PM
This revision is now accepted and ready to land.Mar 28 2022, 11:23 PM

Fix missing reference in TOC & fix formatting of tables

This revision was landed with ongoing or failed builds.Mar 31 2022, 10:38 AM
This revision was automatically updated to reflect the committed changes.
jgorbe added a subscriber: jgorbe.Mar 31 2022, 2:51 PM

Hi, this patch is causing floating point exceptions for us during profile generation. On a debug build I get this assertion failure (see stack trace below):

clang: /usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/BranchProbability.cpp:41: llvm::BranchProbability::BranchProbability(uint32_t, uint32_t): Assertion `Denominator > 0 && "Denominator cannot be 0!"' failed.

Printing some values around the problem I got

TotalBranchWeight = 4294967296                   
LikelyBranchWeight = 2147483648                                                                                                                                                                              
UnlikelyBranchWeight = 2147483648                                                                                                                                                                            
NumUnlikelyTargets = 1

I see the BranchProbability constructor takes uint32_ts, so this looks like it's overflowing to 0 (4294967296 is exactly 2**32).

I'm going to revert the patch to unbreak our build. Please let me know if you need more details and I'll try to come up with a reproducer I can share. Here's the stack trace for the assertion.

 #0 0x000000000a7f992a llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/Unix/Signals.inc:565:11
 #1 0x000000000a7f9afb PrintStackTraceSignalHandler(void*) /usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/Unix/Signals.inc:632:1                                                                   
 #2 0x000000000a7f80bb llvm::sys::RunSignalHandlers() /usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/Signals.cpp:102:5                                                                             
 #3 0x000000000a7fa271 SignalHandler(int) /usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/Unix/Signals.inc:407:1                                                                                    
 #4 0x00007ff57cfe2200 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x13200)                                                                                                                          
 #5 0x00007ff57ca678a1 raise ./signal/../sysdeps/unix/sysv/linux/raise.c:50:1                                                                                                                                
 #6 0x00007ff57ca51546 abort ./stdlib/abort.c:81:7                                                                                                                                                           
 #7 0x00007ff57ca5142f get_sysdep_segment_value ./intl/loadmsgcat.c:509:8                                                                                                                                    
 #8 0x00007ff57ca5142f _nl_load_domain ./intl/loadmsgcat.c:970:34                                                                                                                                            
 #9 0x00007ff57ca60222 (/lib/x86_64-linux-gnu/libc.so.6+0x35222)                                                                                                                                             
#10 0x000000000a6cb517 llvm::BranchProbability::BranchProbability(unsigned int, unsigned int) /usr/local/google/home/jgorbe/code/llvm/llvm/lib/Support/BranchProbability.cpp:0:3                             
#11 0x000000000a937a4d llvm::misexpect::verifyMisExpect(llvm::Instruction&, llvm::ArrayRef<unsigned int>, llvm::ArrayRef<unsigned int>) /usr/local/google/home/jgorbe/code/llvm/llvm/lib/Transforms/Utils/MisExpect.cpp:190:54                                                                                                                                                                                            
#12 0x000000000a937ef3 llvm::misexpect::checkBackendInstrumentation(llvm::Instruction&, llvm::ArrayRef<unsigned int>) /usr/local/google/home/jgorbe/code/llvm/llvm/lib/Transforms/Utils/MisExpect.cpp:217:1  
#13 0x000000000a93807b llvm::misexpect::checkExpectAnnotations(llvm::Instruction&, llvm::ArrayRef<unsigned int>, bool) /usr/local/google/home/jgorbe/code/llvm/llvm/lib/Transforms/Utils/MisExpect.cpp:236:1 
#14 0x0000000009e1339c (anonymous namespace)::SampleProfileLoader::generateMDProfMetadata(llvm::Function&) /usr/local/google/home/jgorbe/code/llvm/llvm/lib/Transforms/IPO/SampleProfile.cpp:1755:19         
#15 0x0000000009e10d94 (anonymous namespace)::SampleProfileLoader::emitAnnotations(llvm::Function&) /usr/local/google/home/jgorbe/code/llvm/llvm/lib/Transforms/IPO/SampleProfile.cpp:0:5                    
#16 0x0000000009e1022b (anonymous namespace)::SampleProfileLoader::runOnFunction(llvm::Function&, llvm::AnalysisManager<llvm::Module>*) /usr/local/google/home/jgorbe/code/llvm/llvm/lib/Transforms/IPO/SampleProfile.cpp:2199:5                                                                                                                                                                                          
#17 0x0000000009e0ce8b (anonymous namespace)::SampleProfileLoader::runOnModule(llvm::Module&, llvm::AnalysisManager<llvm::Module>*, llvm::ProfileSummaryInfo*, llvm::CallGraph*) /usr/local/google/home/jgorbe/code/llvm/llvm/lib/Transforms/IPO/SampleProfile.cpp:2113:15                                                                                                                                                
#18 0x0000000009e0b882 llvm::SampleProfileLoaderPass::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) /usr/local/google/home/jgorbe/code/llvm/llvm/lib/Transforms/IPO/SampleProfile.cpp:2229:7
#19 0x000000000c6e35c5 llvm::detail::PassModel<llvm::Module, llvm::SampleProfileLoaderPass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module> >::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) /usr/local/google/home/jgorbe/code/llvm/llvm/include/llvm/IR/PassManagerInternal.h:88:17
#20 0x0000000009a7ee41 llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module> >::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) /usr/local/google/home/jgorbe/code/llvm/llvm/include/llvm/IR/PassManager.h:522:16
#21 0x000000000c5c9ae1 runNewPMPasses(llvm::lto::Config const&, llvm::Module&, llvm::TargetMachine*, unsigned int, bool, llvm::ModuleSummaryIndex*, llvm::ModuleSummaryIndex const*) /usr/local/google/home/jgorbe/code/llvm/llvm/lib/LTO/LTOBackend.cpp:312:3
#22 0x000000000c5c8cfe llvm::lto::opt(llvm::lto::Config const&, llvm::TargetMachine*, unsigned int, llvm::Module&, bool, llvm::ModuleSummaryIndex*, llvm::ModuleSummaryIndex const*, std::vector<unsigned char, std::allocator<unsigned char> > const&) /usr/local/google/home/jgorbe/code/llvm/llvm/lib/LTO/LTOBackend.cpp:373:3
#23 0x000000000c5cba45 llvm::lto::thinBackend(llvm::lto::Config const&, unsigned int, std::function<llvm::Expected<std::unique_ptr<llvm::CachedFileStream, std::default_delete<llvm::CachedFileStream> > > (unsigned int)>, llvm::Module&, llvm::ModuleSummaryIndex const&, llvm::StringMap<std::unordered_set<unsigned long, std::hash<unsigned long>, std::equal_to<unsigned long>, std::allocator<unsigned long> >, llvm::MallocAllocator> const&, llvm::DenseMap<unsigned long, llvm::GlobalValueSummary*, llvm::DenseMapInfo<unsigned long, void>, llvm::detail::DenseMapPair<unsigned long, llvm::GlobalValueSummary*> > const&, llvm::MapVector<llvm::StringRef, llvm::BitcodeModule, llvm::DenseMap<llvm::StringRef, unsigned int, llvm::DenseMapInfo<llvm::StringRef, void>, llvm::detail::DenseMapPair<llvm::StringRef, unsigned int> >, std::vector<std::pair<llvm::StringRef, llvm::BitcodeModule>, std::allocator<std::pair<llvm::StringRef, llvm::BitcodeModule> > > >*, std::vector<unsigned char, std::allocator<unsigned char> > const&)::$_6::operator()(llvm::Module&, llvm::TargetMachine*, std::unique_ptr<llvm::ToolOutputFile, std::default_delete<llvm::ToolOutputFile> >) const /usr/local/google/home/jgorbe/code/llvm/llvm/lib/LTO/LTOBackend.cpp:594:13
#24 0x000000000c5cb971 llvm::lto::thinBackend(llvm::lto::Config const&, unsigned int, std::function<llvm::Expected<std::unique_ptr<llvm::CachedFileStream, std::default_delete<llvm::CachedFileStream> > > (unsigned int)>, llvm::Module&, llvm::ModuleSummaryIndex const&, llvm::StringMap<std::unordered_set<unsigned long, std::hash<unsigned long>, std::equal_to<unsigned long>, std::allocator<unsigned long> >, llvm::MallocAllocator> const&, llvm::DenseMap<unsigned long, llvm::GlobalValueSummary*, llvm::DenseMapInfo<unsigned long, void>, llvm::detail::DenseMapPair<unsigned long, llvm::GlobalValueSummary*> > const&, llvm::MapVector<llvm::StringRef, llvm::BitcodeModule, llvm::DenseMap<llvm::StringRef, unsigned int, llvm::DenseMapInfo<llvm::StringRef, void>, llvm::detail::DenseMapPair<llvm::StringRef, unsigned int> >, std::vector<std::pair<llvm::StringRef, llvm::BitcodeModule>, std::allocator<std::pair<llvm::StringRef, llvm::BitcodeModule> > > >*, std::vector<unsigned char, std::allocator<unsigned char> > const&) /usr/local/google/home/jgorbe/code/llvm/llvm/lib/LTO/LTOBackend.cpp:670:3
#25 0x000000000ad17e95 runThinLTOBackend(clang::DiagnosticsEngine&, llvm::ModuleSummaryIndex*, llvm::Module*, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, clang::BackendAction) /usr/local/google/home/jgorbe/code/llvm/clang/lib/CodeGen/BackendUtil.cpp:1670:11
#26 0x000000000ad174c8 clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::StringRef, llvm::Module*, clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) /usr/local/google/home/jgorbe/code/llvm/clang/lib/CodeGen/BackendUtil.cpp:1711:9
#27 0x000000000bbd802f clang::CodeGenAction::ExecuteAction() /usr/local/google/home/jgorbe/code/llvm/clang/lib/CodeGen/CodeGenAction.cpp:1209:3
#28 0x000000000b9f1368 clang::FrontendAction::Execute() /usr/local/google/home/jgorbe/code/llvm/clang/lib/Frontend/FrontendAction.cpp:1036:7
#29 0x000000000b9206ab clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /usr/local/google/home/jgorbe/code/llvm/clang/lib/Frontend/CompilerInstance.cpp:1036:23
#30 0x000000000bbc34b9 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /usr/local/google/home/jgorbe/code/llvm/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:266:8
#31 0x0000000006c0de77 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /usr/local/google/home/jgorbe/code/llvm/clang/tools/driver/cc1_main.cpp:248:13
#32 0x0000000006c0076b ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) /usr/local/google/home/jgorbe/code/llvm/clang/tools/driver/driver.cpp:317:5
#33 0x0000000006bff778 main /usr/local/google/home/jgorbe/code/llvm/clang/tools/driver/driver.cpp:388:5
#34 0x00007ff57ca527fd __libc_start_main ./csu/../csu/libc-start.c:332:16
#35 0x0000000006bfef3a _start (/usr/local/google/home/jgorbe/code/llvm-build/bin/clang+0x6bfef3a)

By the way, the line number for llvm::misexpect::verifyMisExpect in the stack trace above includes the debug printfs I inserted to get the variable values so it won't match the exact line number in the repo. But I think that's the only call to BranchProbability in that function so I hope it's not very confusing anyway. Sorry about that.

@jgorbe Sorry for the trouble. Thank you for the backtrace and the revert. Indeed, it seems like I've missed an assumption w.r.t. over/underflow, and will have to sort that out before re-landing.

paulkirth reopened this revision.Apr 1 2022, 3:03 PM
This revision is now accepted and ready to land.Apr 1 2022, 3:03 PM
paulkirth updated this revision to Diff 419931.Apr 1 2022, 10:08 PM

Replace direct use of BranchProbability constructor w/ getBranchProbability().

getBranchProbability() already accounts for and scales branch probability
correctly using uint64_t parameters to fit into a 32-bit integer. This
should avoid any problems w/ overflow when using misexpect.

paulkirth updated this revision to Diff 419933.Apr 1 2022, 10:12 PM

Remove commented out code

tejohnson added inline comments.Apr 4 2022, 9:55 PM
llvm/lib/Transforms/Utils/MisExpect.cpp
151

Nit: seems more intuitive to pass low before high. But not sure the low is needed in this case, see comment at callsite.

202

Looks like Tolerance is unsigned, as are the arguments to clamp. So it can never go below 0.

paulkirth updated this revision to Diff 420397.Apr 5 2022, 12:16 AM

Swap parameters in clamp routine

paulkirth marked an inline comment as done.

rebase

lgtm if you are waiting for a re-review. As noted in my previous comment, you have added a little bit of unnecessary checking at the lower end with your new clamp function, since it is an unsigned value and can never go below 0. But it isn't a big deal if you prefer to keep that in to make it easier to transition to std::clamp.

Ah, thanks. I've had to try to re-land this so many times, I wanted to be sure the pre-submit was looking OK after rebasing. or at least be sure it didn't look like it was failing from any of my changes.

w.r.t. clamp, keeping it compatible was my intent.

This revision was landed with ongoing or failed builds.Apr 19 2022, 2:24 PM
This revision was automatically updated to reflect the committed changes.
bgraur added a subscriber: bgraur.Apr 22 2022, 10:56 AM
hans added a subscriber: hans.Apr 25 2023, 5:05 AM

We gave this a try in Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1434989

While the diagnostics are interesting, two things make this less useful for our developers:

  1. It also fires in situations where the developer didn't set any expectations, for example for static variable initialization guards where clang implicitly adds branch weights (https://crbug.com/1434989#c7)
  2. Due to inlining etc., it often gets the source locations wrong, which means it points at code where again there were no expectations -- but perhaps that code got inlined into an expectations somewhere else. (e.g. https://crbug.com/1434989#c9)

Especially 2) is probably not easy to fix. Do you have any tips on how developers can use this more effectively?

clang/include/clang/Driver/Options.td
1428

Should this be a driver mode option and not just cc1? The doc suggests using it, which currently won't work without an -Xclang prefix.

Thanks for the report Hans.

We gave this a try in Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1434989

While the diagnostics are interesting, two things make this less useful for our developers:

  1. It also fires in situations where the developer didn't set any expectations, for example for static variable initialization guards where clang implicitly adds branch weights (https://crbug.com/1434989#c7)

Hmm, I wasn't even aware that clang would insert llvm.expect without a user annotation. I'm also a bit surprised to see this crop up, since we haven't run into it on Fuchsia.

The problem is that we can't really distinguish those cases in IR... and even when we could, for IR based instrumentation we'd need to embed that into the branch weights somehow. D131306 explores a way to track the provenance of branch weights, so we could probably use that to distinguish them if we could also distinguish the llvm.expect usages the compiler inserts from user inserted ones. I put that work on hold, since it needs an RFC and I've been busy w/ other things, but I'll take some time in the next day or two to write that out and post the RFC to discord, since D131306 should solve a big part of that issue.

  1. Due to inlining etc., it often gets the source locations wrong, which means it points at code where again there were no expectations -- but perhaps that code got inlined into an expectations somewhere else. (e.g. https://crbug.com/1434989#c9)

The checking depends somewhat on the instrumentation type (frontend vs. backend instrumentation) In the case of frontend instrumentation, when the expect intrinsic is lowered (very early in the pipeline) we can report the diagnostic right away, since branch weights from profiling have already been attached. The should mean that you should get fairly accurate source information, since this happens before any optimizations run.

Backend instrumentation is more complicated, since we can't report the discrepancy until we're adding the branch weights to the IR. Whether inlining plays a role there or not is highly dependent on how you're compiling (i.e., no LTO, LTO, ThinLTO). The ordering is also somewhat dependent on if you're using Sampling or Instrumentation based profiles, but I believe both of those will always add weights before inlining. ThinLTO may be an outlier here, since I think weights from sampling based profiles can run multiple times in the ThinLTO pipeline.

Especially 2) is probably not easy to fix. Do you have any tips on how developers can use this more effectively?

When we report the diagnostic, we do our best to provide the source location, but that is only as good as what is available in the IR. I can certainly take a look at how we could improve reporting, but my recollection is that source information isn't always maintained well as the various passes run.If we're lucky in this case, we can maybe just tweak how its reporting the source location to include the inlining context. There's probably a bigger discussion that needs to be had about how to preserve debug and source location info through the backend passes.

The log also reported several places where expected hot code was never executed. If that isn't desirable, I'd also suggest that you could use the -fdiagnostic-hotness-threshold= to ignore reporting about branches that are not executed some minimum number of times. MisExpect is remarks based, so I beleive that is currently working. If not I'm happy to add that functionality.

clang/include/clang/Driver/Options.td
1428

That should also work from clang w/o -Xclang, shouldn't it? This is used exactly like -fdiagnostics-hotness-threshold=, so IIRC that should still work from clang.

hans added a comment.Apr 25 2023, 11:02 AM
  1. Due to inlining etc., it often gets the source locations wrong, which means it points at code where again there were no expectations -- but perhaps that code got inlined into an expectations somewhere else. (e.g. https://crbug.com/1434989#c9)

The checking depends somewhat on the instrumentation type (frontend vs. backend instrumentation) In the case of frontend instrumentation, when the expect intrinsic is lowered (very early in the pipeline) we can report the diagnostic right away, since branch weights from profiling have already been attached. The should mean that you should get fairly accurate source information, since this happens before any optimizations run.

We use the backend instrumentation since that gives the best performance, but it's good to hear that the locations may be more accurate with frontend instrumentation. Maybe we should add that to the docs?

The log also reported several places where expected hot code was never executed. If that isn't desirable, I'd also suggest that you could use the -fdiagnostic-hotness-threshold= to ignore reporting about branches that are not executed some minimum number of times. MisExpect is remarks based, so I beleive that is currently working. If not I'm happy to add that functionality.

Oh nice, I will try that flag.

clang/include/clang/Driver/Options.td
1428

Hmm, if I add -fdiagnostics-misexpect-tolerance=10 I get errors about the flag being unused. I had to use -Xclang for it to work.

  1. Due to inlining etc., it often gets the source locations wrong, which means it points at code where again there were no expectations -- but perhaps that code got inlined into an expectations somewhere else. (e.g. https://crbug.com/1434989#c9)

The checking depends somewhat on the instrumentation type (frontend vs. backend instrumentation) In the case of frontend instrumentation, when the expect intrinsic is lowered (very early in the pipeline) we can report the diagnostic right away, since branch weights from profiling have already been attached. The should mean that you should get fairly accurate source information, since this happens before any optimizations run.

We use the backend instrumentation since that gives the best performance, but it's good to hear that the locations may be more accurate with frontend instrumentation. Maybe we should add that to the docs?

Let me look into this a bit. I'm not sure if this is just we're not doing the best practice when printing the diagnostic, or if the resolution is being degraded due to optimizations. We may want to update the documentation anyway, but I'd like to be certain.

clang/include/clang/Driver/Options.td
1428

That isn't WAI then. I'll try to address that today.