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

Tue Apr 25 11:33:11 PDT 2023

paulkirth added a comment.

In D115907#4296154 <https://reviews.llvm.org/D115907#4296154>, @hans wrote:

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

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.

================
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:
> 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.
That isn't WAI then. I'll try to address that today.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115907