[PATCH] D66324: clang-misexpect: Profile Guided Validation of Performance Annotations in LLVM

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Missing tests for `__builtin_unpredictable()`.



================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:626
+  unsigned Line, Column;
+  bool BadDebugInfo = false;
+  FullSourceLoc Loc =
----------------
paulkirth wrote:
> lebedev.ri wrote:
> > This should be `BadDebugInfoLoc` (in `getBestLocationFromDebugLoc()` too)
> That should be in a separate patch, right?
Yeah, let's keep that for later.


================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:369
     void UnsupportedDiagHandler(const llvm::DiagnosticInfoUnsupported &D);
+    void MisExpectDiagHandler(const llvm::DiagnosticInfoMisExpect &D);
     /// Specialized handlers for optimization remarks.
----------------
Comment missing


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3449
 
+  if (Diags.isIgnored(diag::warn_profile_data_misexpect, SourceLocation()))
+    Res.FrontendOpts.LLVMArgs.push_back("-pgo-warn-misexpect");
----------------
Err, this works? I'd expect `!` to be missing here..


================
Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:78-87
+  SI.setMetadata(
+      LLVMContext::MD_misexpect,
+      MDBuilder(CI->getContext())
+          .createMisExpect(Index, LikelyBranchWeight, UnlikelyBranchWeight));
+
+  SI.setCondition(ArgValue);
+  misexpect::checkClangInstrumentation(SI);
----------------
paulkirth wrote:
> lebedev.ri wrote:
> > Why can't `LLVMContext::MD_prof` be reused?
> It didn't seem appropriate to me, but that can be changed if it is the right thing to do. 
> 
> However, I don't see code elsewhere that loops over `MD_prof` metadata checking its tag. I see lots of examples that check if the tag matches, and then exits if it doesn't match. 
> 
> If I added "misexpect" as a new `MD_prof`tag, then code in places like `extractProfTotalWeight(...)` in IR/Metadata.cpp would have to be updated. I'm not sure how pervasive that pattern is, but I did  want to avoid introducing systemic changes like that.
> 
> https://github.com/llvm/llvm-project/blob/9976a5bc1db5a240378a61a68c0e182060bcb554/llvm/lib/IR/Metadata.cpp#L1336
That's not exactly what i was asking.
I'm specifically talking about how `"misexpect"` and `"branch_weigths"` carry basically the same data,
are set basically in the same place always, with the difference that `"misexpect"` also has the switch case index.
This is both somewhat bloaty, and is prone to getting out of sync.

I have two questions:
1. Can `"misexpect"` metadata simply not be invented, but can existing `LLVMContext::MD_misexpect` simply be used?
2. Can `"misexpect"` be changed to not store the weights, but a reference to the `LLVMContext::MD_misexpect` metadata that will be emitted anyway?



================
Comment at: llvm/test/Transforms/LowerExpectIntrinsic/basic.ll:141
   %tobool = icmp ne i64 %expval, 0
-; CHECK: !prof !1
+; CHECK: !prof !2
 ; CHECK-NOT: @llvm.expect
----------------
Here and elsewhere:
is `!prof !1` no longer present?


================
Comment at: llvm/test/Transforms/LowerExpectIntrinsic/basic.ll:292-293
+; CHECK: !3 = !{!"misexpect", i64 1, i64 2000, i64 1}
+; CHECK: !4 = !{!"branch_weights", i32 1, i32 2000, i32 1}
+; CHECK: !5 = !{!"branch_weights", i32 2000, i32 1, i32 1}
----------------
Should there be a FIXME, are some `misexpect` missing here?


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

https://reviews.llvm.org/D66324