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

Tue Mar 15 23:14:40 PDT 2022

tejohnson added inline comments.

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

================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1967
+        Diags.Report(diag::warn_drv_diagnostics_misexpect_requires_pgo)
+            << "-fdiagnostics-misexpect-tollerance=";
+    }
----------------
Add tests for expected errors?

================
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);
----------------
Can you add a case where an unlikely() is wrong?

================
Comment at: llvm/docs/MisExpect.rst:2
+===================
+Misexpect
+===================
----------------
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.

================
Comment at: llvm/docs/MisExpect.rst:22
+The MisExpect checks in the LLVM backend follow a simple procedure: if the
+profiling counter associated with an instruction using the ``llvm.expect``
+intrinsic was too low along the expected path, then it emits a diagnostic
----------------
paulkirth wrote:
> tejohnson wrote:
> > Suggest using "profile weight" not profiling counter since we don't really have profile counters associated with instructions (rather with edges in the MST, at least in the IR PGO).
> > 
> > Also, doesn't the profile weight being too low only handle the LIKELY case? What about something being marked UNLIKELY with a hot/high profile weight? edit: After looking through the implementation, I think the comparison only being done in this direction is based on the way it is implemented, but it is unclear from this documentation here how the comparison is handling things being off in either direction.
> Maybe my mental model is off here, but doesn't the `llvm.expect` intrinsic mark a specific value as the 'likely' or expected value? So if you want to mark a branch as unlikely, you're essentially marking the other half of it as 'hot'.
> 
> We could change the comparison to compare all parts of the branch weights too, but the case of an 'unlikely' branch being too hot in my model is captured by the 'likely' branch becoming too 'cold'.
> 
> If that is incorrect, I'd really appreciate some guidance on how to model this more accurately.
Yes I agree that is how llvm.expect is implemented, and I can see from your implementation that it should be handling that correctly. It was just unclear from the way it is written here what was happening. I like the new wording which is less specific to the way the checking is implemented.

================
Comment at: llvm/docs/MisExpect.rst:39
+``-unlikely-branch-weight`` LLVM options. During verification, if the
+profile count is less than the calculated threshold, then we will emit a
+remark or warning detailing a potential performance regression. The
----------------
paulkirth wrote:
> tejohnson wrote:
> > s/count/weight/
> > 
> > Also, similar to my earlier comment, what about an expect UNLIKELY branch with a high profile weight?
> I think I've reworded this to clarify things a bit more, but let me know if it still needs some polish.
I think the new version is good, thanks.

================
Comment at: llvm/include/llvm/Transforms/Utils/MisExpectTolleranceParser.h:12
+/// This file implements a simple parser to decode commandline option for
+/// misexpect tollerance that supports both int.
+///
----------------
Is the end of the sentence incomplete? I.e. should be "both int and (something else)"?

================
Comment at: llvm/include/llvm/Transforms/Utils/MisExpectTolleranceParser.h:26
+// Parse misexpect tollerance argument value.
+// Valid option values are
+// integer: manually specified threshold
----------------
Fix line wrapping. But sentence could be simplified too - is this just saying that the input must be an integer?

================
Comment at: llvm/include/llvm/Transforms/Utils/MisExpectTolleranceParser.h:42
+// A simple CL parser for '-misexpect-tollerance='
+class HotnessThresholdParser : public cl::parser<Optional<uint64_t>> {
+public:
----------------
Where is this class used?

================
Comment at: llvm/include/llvm/Transforms/Utils/MisExpectTolleranceParser.h:51
+      return O.error("Invalid argument '" + Arg +
+                     "', only integer or 'auto' is supported.");
+
----------------
I don't see where 'auto' is handled?

================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1737

+    misexpect::checkExpectAnnotations(*TI, Weights, false);
+
----------------
Document const parameter (e.g. "/*IsFrontend=*/false"), here and in other calls.

================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:78
+
+Instruction *getOprndOrInst(Instruction *I) {
+  assert(I != nullptr && "MisExpect target Instruction cannot be nullptr");
----------------
Suggest renaming to something more intuitive. E.g. getInstCondition? 

================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:180
+  // To determine our threshold value we need to obtain the branch probability
+  // for the weights added by llvm.expect and use that proportion to calcualte
+  // our threshold based on the colledted profile data.
----------------
typo in calculate

================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:181
+  // for the weights added by llvm.expect and use that proportion to calcualte
+  // our threshold based on the colledted profile data.
+  const llvm::BranchProbability LikelyProbablilty(LikelyBranchWeight,
----------------
typo in collected

================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:188
+  auto Tollerance = getMisExpectTollerance(I.getContext());
+  if (Tollerance < 0)
+    Tollerance = 0;
----------------
afaict this handling duplicates what is done in parseTolleranceOption when the tolerance comes in from the clang option. Can you just do this handling once, here?

================
Comment at: llvm/test/Transforms/PGOProfile/misexpect-branch-correct.ll:1
+; RUN: llvm-profdata merge %S/Inputs/misexpect-branch-correct.proftext -o %t.profdata
+
----------------
Add a one sentence description at the top of the llvm tests about what they are testing.

================
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
+
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115907