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

[Paul Kirth via Phabricator via llvm-commits]

paulkirth marked 10 inline comments as done.
paulkirth added inline comments.


================
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);
----------------
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. 


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


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


================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:78
+
+Instruction *getOprndOrInst(Instruction *I) {
+  assert(I != nullptr && "MisExpect target Instruction cannot be nullptr");
----------------
tejohnson wrote:
> Suggest renaming to something more intuitive. E.g. getInstCondition? 
That's definitely a better name. Thanks for the suggestion.


================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:188
+  auto Tollerance = getMisExpectTollerance(I.getContext());
+  if (Tollerance < 0)
+    Tollerance = 0;
----------------
tejohnson wrote:
> 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?
This is now the only place we check/modify the value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115907