[misexpect] Support missing-annotations diagnostics from frontend profile data

[ilovepi]

Emitting diagnostics for frontend instrumentation has to be done
differently, since in the LowerExpectIntrinsic pass, the profile data
has already been attached to an instruction, which is the revers of the
case for IR PGO. This patch extends the implementation to support
frontend profiles in LowerExpectIntrinsic.

[llvmbot]

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-pgo

@llvm/pr-subscribers-llvm-transforms

Author: Paul Kirth (ilovepi)

Changes

---

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

3 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp (+18)
• (modified) llvm/lib/Transforms/Utils/MisExpect.cpp (+1-2)
• (modified) llvm/test/Transforms/PGOProfile/missing-annotation.ll (+1-1)

diff --git a/llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp b/llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
index 17c5a4ee1fd0b..4075749d0d574 100644
--- a/llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
+++ b/llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
@@ -369,9 +369,21 @@ static bool lowerExpectIntrinsic(Function &F) {
     if (BranchInst *BI = dyn_cast<BranchInst>(BB.getTerminator())) {
       if (handleBranchExpect(*BI))
         ExpectIntrinsicsHandled++;
+      else {
+        SmallVector<uint32_t> Weights;
+        if (extractBranchWeights(*BI, Weights))
+          misexpect::checkMissingAnnotations(*BI, Weights,
+                                             /*IsFrontendInstr=*/false);
+      }
     } else if (SwitchInst *SI = dyn_cast<SwitchInst>(BB.getTerminator())) {
       if (handleSwitchExpect(*SI))
         ExpectIntrinsicsHandled++;
+      else {
+        SmallVector<uint32_t> Weights;
+        if (extractBranchWeights(*SI, Weights))
+          misexpect::checkMissingAnnotations(*SI, Weights,
+                                             /*isFrontend=*/false);
+      }
     }
 
     // Remove llvm.expect intrinsics. Iterate backwards in order
@@ -383,6 +395,12 @@ static bool lowerExpectIntrinsic(Function &F) {
         if (SelectInst *SI = dyn_cast<SelectInst>(&Inst)) {
           if (handleBrSelExpect(*SI))
             ExpectIntrinsicsHandled++;
+          else {
+            SmallVector<uint32_t> Weights;
+            if (extractBranchWeights(*SI, Weights))
+              misexpect::checkMissingAnnotations(*SI, Weights,
+                                                 /*isFrontend=*/false);
+          }
         }
         continue;
       }
diff --git a/llvm/lib/Transforms/Utils/MisExpect.cpp b/llvm/lib/Transforms/Utils/MisExpect.cpp
index 933d9a146533d..1d88f867971e8 100644
--- a/llvm/lib/Transforms/Utils/MisExpect.cpp
+++ b/llvm/lib/Transforms/Utils/MisExpect.cpp
@@ -302,8 +302,7 @@ void checkMissingAnnotations(Instruction &I,
     return;
 
   if (IsFrontendInstr) {
-    // TODO: Frontend checking will have to be thought through, since we need
-    // to do the check on branches that don't have expect intrinsics
+    verifyMissingAnnotations(I, ExistingWeights);
   } else {
     SmallVector<uint32_t> ExpectedWeights;
     if (extractBranchWeights(I, ExpectedWeights))
diff --git a/llvm/test/Transforms/PGOProfile/missing-annotation.ll b/llvm/test/Transforms/PGOProfile/missing-annotation.ll
index 6b52302449900..03b0b3bb5cc54 100644
--- a/llvm/test/Transforms/PGOProfile/missing-annotation.ll
+++ b/llvm/test/Transforms/PGOProfile/missing-annotation.ll
@@ -3,7 +3,7 @@
 
 ; RUN: llvm-profdata merge %S/Inputs/misexpect-branch-correct.proftext -o %t.profdata
 
-; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-missing-annotations -pass-remarks=missing-annotation -S 2>&1 | FileCheck %s --check-prefix=MISSING_ANNOTATION
+; RUN: opt < %s -passes="function(lower-expect),pgo-instr-use" -pgo-test-profile-file=%t.profdata -pgo-missing-annotations -pass-remarks=missing-annotations -S 2>&1 | FileCheck %s --check-prefix=MISSING_ANNOTATION
 
 ; MISSING_ANNOTATION: remark: misexpect-branch.c:22:0:  Extremely hot condition. Consider adding llvm.expect intrinsic
 

[teresajohnson]

Confused about how this relates to enabling checking for frontend instrumentation since all of these set that flag to false?

[ilovepi]

oh no! You're right, this is basically dead. Let me go through this and try to figure out why I thought this was working as intended.

[teresajohnson]

Can you clarify how this change relates to the new functionality in this PR?

Also, the description of this test seems wrong? There are also some "CHECK" labels further below that don't have a corresponding FileCheck invocation. Looks like this might be leftover from a different test this is based off of.

[ilovepi]

Oops, I thought I had removed those in the base revision. Let me rebase this stack and address that in #96523.

[ellishg]

Can we avoid creating a new .proftext file and instead use split-file so that the contents of the test is in one place?
https://llvm.org/docs/TestingGuide.html#extra-files

[ilovepi]

Sure. The use of Inputs here is mostly following the convention I've seen in other tests. I'm unsure how much I can reuse the .proftext file between tests, so I can probably just use split-lines for those too.

[ellishg]

If there is only one FileCheck, I would prefer to use the default CHECK prefix.

[ilovepi]

Noted. I'm considering adding more checks, but I can address that