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

Tue Apr 25 11:02:27 PDT 2023

hans added a comment.

In D115907#4295923 <https://reviews.llvm.org/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 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.

================
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. ">;
----------------
paulkirth wrote:
> 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.
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115907