[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth added a comment. In D115907#4296154 , @hans wrote: > In D115907#4295923 , @paulkirth > wrote: > >>> 2. Due to inlining etc., it often gets the source locations wrong, which >>> means it points at code

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

hans added a comment. In D115907#4295923 , @paulkirth wrote: >> 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

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth added a comment. Thanks for the report Hans. In D115907#4295363 , @hans wrote: > 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

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

hans added a comment. Herald added subscribers: hoy, Enna1, StephenFan. 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

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth added a comment. 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.

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

tejohnson added a comment. 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

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

tejohnson added inline comments. Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:150 +// TODO: when clang allows c++17, use std::clamp instead +uint32_t clamp(uint64_t value, uint32_t hi, uint32_t low) { + if (value > hi) Nit: seems more intuitive to pass

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth added a comment. @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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

jgorbe added a comment. By the way, the line number for `llvm::misexpect::verifyMisExpect` in the stack trace above includes the debug `printf`s 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

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

jgorbe added a comment. 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:

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG46774df30715: [misexpect] Re-implement MisExpect Diagnostics (authored by paulkirth). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth updated this revision to Diff 419181. paulkirth added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115907/new/ https://reviews.llvm.org/D115907 Files: clang/docs/MisExpect.rst clang/docs/ReleaseNotes.rst

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth updated this revision to Diff 418924. paulkirth added a comment. Herald added a subscriber: arphaman. Fix missing reference in TOC & fix formatting of tables Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115907/new/

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth added a comment. reverted. I will fix this tomorrow. Sorry for the trouble Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115907/new/ https://reviews.llvm.org/D115907 ___ cfe-commits mailing

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

tonic added a comment. 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

This revision was automatically updated to reflect the committed changes. Closed by commit rG2add3fbd976d: [misexpect] Re-implement MisExpect Diagnostics (authored by paulkirth). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115907/new/

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

tejohnson added a comment. In D115907#3412685 , @paulkirth wrote: > @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

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth added a comment. @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? Repository: rG LLVM Github Monorepo

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

tejohnson added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1554 + Twine(*Opts.DiagnosticsMisExpectTolerance), SA); + for (StringRef Sanitizer : serializeSanitizerKinds(Opts.SanitizeRecover)) paulkirth wrote: > I really

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth updated this revision to Diff 418328. paulkirth added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115907/new/ https://reviews.llvm.org/D115907 Files: clang/docs/MisExpect.rst clang/docs/ReleaseNotes.rst

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth updated this revision to Diff 418280. paulkirth added a comment. 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. Repository:

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1554 + Twine(*Opts.DiagnosticsMisExpectTolerance), SA); + for (StringRef Sanitizer : serializeSanitizerKinds(Opts.SanitizeRecover)) I really don't understand

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth updated this revision to Diff 418025. paulkirth added a comment. Add missing GenerateArgs call to propagate flags to the backend outside of cc1 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115907/new/ https://reviews.llvm.org/D115907

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

fhahn added a comment. 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) Repository: rG LLVM Github

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth added a comment. These pass for me locally, so I'm reverting for now and will dig into this tomorrow. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115907/new/ https://reviews.llvm.org/D115907

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

thakis added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115907/new/

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe7749d4713a5: [misexpect] Re-implement MisExpect Diagnostics (authored by paulkirth). Changed prior to commit:

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth marked 5 inline comments as done. paulkirth added inline comments. Comment at: llvm/docs/MisExpect.rst:54 + +.. option:: -pgo-warn-misexpect + tejohnson wrote: > paulkirth wrote: > > tejohnson wrote: > > > Looking at the code, the -pgo-warn-misexpect

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth updated this revision to Diff 416356. paulkirth marked 4 inline comments as done. paulkirth added a comment. 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

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

xbolva00 added a comment. Add Clang release note as well please. Comment at: clang/docs/MisExpect.rst:58 + Relaxes misexpect checking to tolerate profiling values within N% of the + expected branch weight. e.g., a vlaue of ``N=5`` allows misexpect to check against +

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. lgtm Just some minor cleanup before submitting as noted below. Comment at: clang/test/Profile/misexpect-branch.c:3 + +// test likely branch +// RUN: llvm-profdata

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth marked 10 inline comments as done. paulkirth added inline comments. Comment at: clang/test/Profile/misexpect-branch.c:25 + int x = 0; + if (likely(rando % (outer_loop * inner_loop) == 0)) { // exact-warning-re {{Potential performance regression from use of

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth updated this revision to Diff 415990. paulkirth added a comment. 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. Repository: rG LLVM Github

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

tejohnson added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.h:425 + /// values in order to be included in misexpect diagnostics. + Optional DiagnosticsMisExpectTollerance = 0; + s/Tollerance/Tolerance/ (and elsewhere throughout patch)

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth added a comment. 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

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

xbolva00 added a comment. Please also consider https://github.com/llvm/llvm-project/issues/46534 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115907/new/ https://reviews.llvm.org/D115907 ___

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth marked 6 inline comments as done. paulkirth added inline comments. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:1203 + if (CodeGenOpts.MisExpect) { +Ctx.setMisExpectWarningRequested(true); tejohnson wrote: > Out of curiosity, since I am less

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth updated this revision to Diff 414230. paulkirth added a comment. Consolodate common code, clarify documentation, and address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115907/new/ https://reviews.llvm.org/D115907 Files:

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

tejohnson added inline comments. Herald added a project: All. Comment at: clang/include/clang/Basic/CodeGenOptions.def:172 CODEGENOPT(NoWarn, 1, 0) ///< Set when -Wa,--no-warn is enabled. +CODEGENOPT(MisExpect , 1, 0) ///< Set -Wmisexpect is enabled

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115907/new/ https://reviews.llvm.org/D115907 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth updated this revision to Diff 395727. paulkirth added a comment. - [misexpect] Add missing tests and modify diagnostic Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115907/new/ https://reviews.llvm.org/D115907 Files:

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth updated this revision to Diff 395517. paulkirth added a comment. - [misexpect] Add missing tests and modify diagnostic Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115907/new/ https://reviews.llvm.org/D115907 Files:

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth updated this revision to Diff 395502. paulkirth added a comment. - [misexpect] Add missing tests and modify diagnostic Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115907/new/ https://reviews.llvm.org/D115907 Files:

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

paulkirth created this revision. Herald added subscribers: abrachet, ormris, dexonsmith, wenlei, phosek, hiraditya, mgorny. paulkirth updated this revision to Diff 394995. paulkirth added a comment. paulkirth updated this revision to Diff 394997. paulkirth added reviewers: phosek, mcgrathr,