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

Tue Apr 25 09:20:23 PDT 2023

paulkirth added a comment.

Thanks for the report Hans.

In D115907#4295363 <https://reviews.llvm.org/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 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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D131306> should solve a big part of that issue.

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

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.

================
Comment at: clang/include/clang/Driver/Options.td:1434
+def fdiagnostics_misexpect_tolerance_EQ : Joined<["-"], "fdiagnostics-misexpect-tolerance=">,
+    Group<f_Group>, Flags<[CC1Option]>, MetaVarName<"<value>">,
+    HelpText<"Prevent misexpect diagnostics from being output if the profile counts are within N% of the expected. ">;
----------------
hans wrote:
> 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.
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115907