Page MenuHomePhabricator

[misexpect] [WIP] Enable diagnostics for profitable llvm.expect annotations
Needs ReviewPublic

Authored by paulkirth on Jul 15 2022, 11:47 AM.

Details

Summary

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

Diff Detail

Unit TestsFailed

TimeTest
60,070 msx64 debian > AddressSanitizer-x86_64-linux.TestCases::scariness_score_test.cpp
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -O0 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/scariness_score_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Output/scariness_score_test.cpp.tmp

Event Timeline

paulkirth created this revision.Jul 15 2022, 11:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2022, 11:47 AM

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 and D128858 before landing this, since those should make the implementation cleaner.
paulkirth updated this revision to Diff 449508.Aug 2 2022, 6:50 PM

Refactor an clean ups

paulkirth updated this revision to Diff 449858.Aug 3 2022, 8:30 PM

Clean up code.

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

paulkirth updated this revision to Diff 450191.Aug 4 2022, 5:24 PM

Small update

paulkirth retitled this revision from [misexpect] Enable diagnostics for profitable llvm.expect annotations to [misexpect] [WIP] Enable diagnostics for profitable llvm.expect annotations.Aug 17 2022, 6:15 PM

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.

llvm/lib/Transforms/Utils/MisExpect.cpp
312–320

This can probably be cleaned up/removed, since D131136 implements support for frontend profiling.

paulkirth added inline comments.Aug 17 2022, 6:30 PM
llvm/lib/Transforms/Utils/MisExpect.cpp
183–189

Needs a rebase, since this is now gone

paulkirth published this revision for review.Aug 17 2022, 6:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2022, 6:37 PM