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

[Paul Kirth via Phabricator via llvm-commits]

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


================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:1203
 
+  if (CodeGenOpts.MisExpect) {
+    Ctx.setMisExpectWarningRequested(true);
----------------
tejohnson wrote:
> Out of curiosity, since I am less familiar with clang than llvm, when is this path taken vs where this is done at line 336 in HandleTranslationUnit?
> 
> Also, it might be nice to have this code placed either consistently before or after the OptRecordFile handling in these two locations, so it is easier to compare.
That was a good catch, I had forgotten to remove this after patching in line 366. I moved that a bit a few lines lower to fit more nicely, and removed this one entirely.


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




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


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


================
Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:109
 
+  SI.setCondition(ArgValue);
+
----------------
tejohnson wrote:
> Is there a reason why this line has changed ordering w.r.t. setMetadata?
I think I actually restored this to what it was before the original MisExpect patch, but there is no need for that, so I've removed it.


================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:170
+
+  if (ProfileCount < ScaledThreshold)
+    emitMisexpectDiagnostic(I, Ctx, ProfileCount, CaseTotal);
----------------
tejohnson wrote:
> Is this too strict - i.e. what if they are off by only a small amount?
Maybe. I don't know what a good default fudge factor would be, though. Maybe use 5%, and expose a flag that can override it?

Another approach is that the values used by `llvm.expect` can be changed via clang flags, which will update the weights checked against. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115907