[PATCH] D129889: [misexpect] [WIP] Enable diagnostics for profitable llvm.expect annotations

[Paul Kirth via Phabricator via llvm-commits]

paulkirth created this revision.
Herald added subscribers: Enna1, wenlei, hiraditya.
Herald added a project: All.
paulkirth added a comment.
paulkirth updated this revision to Diff 449508.
paulkirth updated this revision to Diff 449858.
paulkirth updated this revision to Diff 450191.
paulkirth retitled this revision from "[misexpect] Enable diagnostics for profitable llvm.expect annotations" to "[misexpect] [WIP] Enable diagnostics for profitable llvm.expect annotations".
paulkirth added reviewers: phosek, nickdesaulniers, tejohnson.
paulkirth published this revision for review.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

TODOs

1. Refactor code to live separately from MisExpect.cpp -- alternatively, we could leave them combined and rename the files to PGODiagUtils or something
2. Address Frontend Profiling
3. Support Sample Profiling -- this may be hard until we can track `branch_weight` provenance from `llvm.expect`
4. We should land the cleanups in D128860 <https://reviews.llvm.org/D128860> and D128858 <https://reviews.llvm.org/D128858> before landing this, since those should make the implementation cleaner.


paulkirth added a comment.

Refactor an clean ups


paulkirth added a comment.

Clean up code.

Replace code repitition w/ functions.
Refactor comon functionality to share between diagnostics


paulkirth added a comment.

Small update


paulkirth added a comment.

While WIP, this is far enough along that I'd like to start getting some feedback.

Outstanding Issues:

1. more/better tests (loops!)
2. Should we rename/reorg files+code?
3. audit data types (do we need 64-bit everywhere?)
4. Small code cleanups (MissingAnnotations belongs in it's own namespace)

One thing I'm considering here is if the missing annotation checks/diagnostics should be part of MisExpect or not. One option is to rename `MisExpect.cpp` something like `ProfilingDiags.cpp` or `ExpectDiagnostics.cpp`, and leave these implementations combined. Splitting this up also has some advantages, but then maybe the common bits will need to be extracted into a library.



================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:183-189
+uint32_t clamp(uint64_t Value, uint32_t Low, uint32_t Hi) {
+  if (Value > Hi)
+    return Hi;
+  if (Value < Low)
+    return Low;
+  return Value;
+}
----------------
Needs a rebase, since this is now gone


================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:312-320
+  if (IsFrontendInstr) {
+    // TODO: Fronend checking will have to be thought through, since we need
+    // to do the check on branches that don't have expect intrinsics
+
+    // auto RealWeightsOpt = extractWeights(&I, I.getContext());
+    // if (!RealWeightsOpt)
+    // return;
----------------
This can probably be cleaned up/removed, since D131136 implements support for frontend profiling.


Issue 56502 describes an enhancement related to the use of llvm.expect.
The request is for a diagnostic mode that can identify branches that
would benefit from the use of llvm.expect based on the branch_weights
assigned from a PGO or sample profile.

To support identify branches(or switches) that would benefit from the
use of an llvm.expect intrinsic, we follow a similar checking pattern to
that used in MisExpect, but only in cases where MisExpect diagnostics
would not be used (i.e., when an llvm.expect intrinsic has already been
used).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129889

Files:
  llvm/include/llvm/IR/LLVMContext.h
  llvm/include/llvm/Transforms/Utils/MisExpect.h
  llvm/lib/IR/LLVMContext.cpp
  llvm/lib/IR/LLVMContextImpl.h
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/lib/Transforms/Utils/MisExpect.cpp
  llvm/test/Transforms/PGOProfile/missing-annotation.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D129889.450191.patch
Type: text/x-patch
Size: 17911 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220818/65bc164d/attachment.bin>