[llvm][misexpect] Update MisExpect to use provenance tracking metadata

[ilovepi]

With the IR extension added to MD_prof branch weights, we can now easily
destinguish between weights added by llvm.expect* intrinsics and
weights from other sources. This patch re-enables the assert checking
for malformed metadata, which should never happen using the
llvm.expect* family of intrinsics.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Paul Kirth (ilovepi)

Changes

With the IR extension added to MD_prof branch weights, we can now easily
destinguish between weights added by llvm.expect* intrinsics and
weights from other sources. This patch re-enables the assert checking
for malformed metadata, which should never happen using the
llvm.expect* family of intrinsics.

---

Full diff: https://github.com/llvm/llvm-project/pull/86610.diff

1 Files Affected:

• (modified) llvm/lib/Transforms/Utils/MisExpect.cpp (+10-9)

diff --git a/llvm/lib/Transforms/Utils/MisExpect.cpp b/llvm/lib/Transforms/Utils/MisExpect.cpp
index 759289384ee06d..6d1c26b6cedcb5 100644
--- a/llvm/lib/Transforms/Utils/MisExpect.cpp
+++ b/llvm/lib/Transforms/Utils/MisExpect.cpp
@@ -151,15 +151,9 @@ void verifyMisExpect(Instruction &I, ArrayRef<uint32_t> RealWeights,
   uint64_t TotalBranchWeight =
       LikelyBranchWeight + (UnlikelyBranchWeight * NumUnlikelyTargets);
 
-  // FIXME: When we've addressed sample profiling, restore the assertion
-  //
-  // We cannot calculate branch probability if either of these invariants aren't
-  // met. However, MisExpect diagnostics should not prevent code from compiling,
-  // so we simply forgo emitting diagnostics here, and return early.
-  // assert((TotalBranchWeight >= LikelyBranchWeight) && (TotalBranchWeight > 0)
-  //              && "TotalBranchWeight is less than the Likely branch weight");
-  if ((TotalBranchWeight == 0) || (TotalBranchWeight <= LikelyBranchWeight))
-    return;
+  // Failing this assert means that we have corrupted metadata.
+  assert((TotalBranchWeight >= LikelyBranchWeight) && (TotalBranchWeight > 0)
+               && "TotalBranchWeight is less than the Likely branch weight");
 
   // To determine our threshold value we need to obtain the branch probability
   // for the weights added by llvm.expect and use that proportion to calculate
@@ -186,6 +180,13 @@ void verifyMisExpect(Instruction &I, ArrayRef<uint32_t> RealWeights,
 
 void checkBackendInstrumentation(Instruction &I,
                                  const ArrayRef<uint32_t> RealWeights) {
+  // Backend checking assumes any existing weight comes from an `llvm.expect`
+  // intrinsic. However, SampleProfiling + ThinLTO add branch weights  multiple
+  // times, leading to an invalid assumption in our checking. Backend checks
+  // should only operate on branch weights that carry the "!expected" field,
+  // since they are guaranteed to be added by the LowerExpectIntrinsic pass.
+  if(!hasExpectedProvenance(I))
+    return;
   SmallVector<uint32_t> ExpectedWeights;
   if (!extractBranchWeights(I, ExpectedWeights))
     return;

[ilovepi]

Ping. Now that the metadata patches have landed, I think we can start discussing using that in MisExpect.

[ilovepi]

ping

[david-xl]

Should it be an assert or disagnostic?

[ilovepi]

IIRC many of the places where we check metadata for corruption or format incompatibility just use asserts, but I'm fine if its a diagnostic. Let me know if you have a preference, as I'm fine either way.

That said, if we're going to make this a diagnostic, we should probably move it out of MisExpect and put it on some other code path where we're examining branch weights, since MisExpect is one of the least used compilation modes. WDYT?

[david-xl]

Do we have examples of this assert firing? If it is something user do not have control with, leave it as an assert is fine. If not, exposing it as diagnostic is better.

[ilovepi]

it was introduced after we found an issue w/ thinLTO, since sample profiling can run multiple times. We then introduced an early return to avoid division by zero, but now it should be safe to just use the assert, since we should only ever process branches w/ the provenance tag.

for now, I guess let's leave it as an assert, since I don't expect it to fire, and users don't have control of it.

[llvm-ci]

LLVM Buildbot has detected a new failure on builder arc-builder running on arc-worker while building llvm at step 5 "build-unified-tree".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/3/builds/1343

Here is the relevant piece of the build log for the reference:

Step 5 (build-unified-tree) failure: build (failure)
...
27.558 [5/15/99] Linking CXX shared library lib/libclang.so.19.0.0git
27.565 [4/15/100] Creating library symlink lib/libclang.so.19.0git lib/libclang.so
27.646 [2/16/101] Linking CXX executable bin/c-arcmt-test
27.944 [2/15/102] Linking CXX executable bin/llvm-dwp
28.269 [2/14/103] Linking CXX executable bin/clang-scan-deps
28.333 [2/13/104] Linking CXX executable bin/llvm-gsymutil
28.379 [2/12/105] Linking CXX executable bin/llvm-isel-fuzzer
28.496 [2/11/106] Linking CXX executable bin/clang-check
28.644 [2/10/107] Linking CXX executable bin/llvm-exegesis
29.519 [2/9/108] Linking CXX executable bin/llvm-split
command timed out: 1200 seconds without output running [b'ninja', b'-j', b'16'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=1429.349399