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

[Teresa Johnson via Phabricator via llvm-commits]

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 merge %S/Inputs/misexpect-branch.proftext -o %t.profdata
----------------
Comment is a little unclear, as it is testing both likely and unlikely branches.


================
Comment at: clang/test/Profile/misexpect-branch.c:9
+// there should be no diagnostics when the tolerance is sufficiently high, or when -Wmisexpect is not requested
+//
+// RUN: %clang_cc1 %s -O2 -o - -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify=foo -fdiagnostics-misexpect-tolerance=10 -Wmisexpect -debug-info-kind=line-tables-only
----------------
nit: stray empty comment line?


================
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 __builtin_expect(): Annotation was correct on {{.+}}% ({{[0-9]+ / [0-9]+}}) of profiled executions.}}
+    x = baz(rando);
----------------
paulkirth wrote:
> tejohnson wrote:
> > Can you add a case where an unlikely() is wrong?
> 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. 
Right, it is just for completeness to test / illustrate that the handling works in both directions.


================
Comment at: llvm/docs/MisExpect.rst:2
+===================
+Misexpect
+===================
----------------
paulkirth wrote:
> tejohnson wrote:
> > paulkirth wrote:
> > > tejohnson wrote:
> > > > 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?
> > > 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.
> > > 
> > > 
> > Should the clang documentation already be added to this patch? I couldn't find it.
> 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?
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.


================
Comment at: llvm/docs/MisExpect.rst:54
+
+.. option:: -pgo-warn-misexpect
+
----------------
paulkirth wrote:
> tejohnson wrote:
> > 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.
> 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.
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.


================
Comment at: llvm/include/llvm/Transforms/Utils/MisExpectToleranceParser.h:27
+// Valid option values are integers in the range [0, 100)
+inline Expected<Optional<uint64_t>> parseToleranceOption(StringRef Arg) {
+  int64_t Val;
----------------
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.


================
Comment at: llvm/lib/IR/LLVMContextImpl.h:1384
+  /// The percentage of difference between profiling branch weights and
+  // llvm.expect branch weights to tollerate when emiting MisExpect diagnostics
+  Optional<uint64_t> DiagnosticsMisExpectTolerance = 0;
----------------
s/tollerate/tolerate/


================
Comment at: llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll:3
+
+; RUN: opt < %s -lower-expect -pgo-instr-use -pgo-test-profile-file=%t.profdata -S -pgo-warn-misexpect -pass-remarks=misexpect 2>&1 | FileCheck %s
+
----------------
tejohnson wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115907