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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added inline comments.


================
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");
----------------
lebedev.ri wrote:
> Err, this works? I'd expect `!` to be missing here..
I ran check-clang and check-llvm. Are there more tests I should be running?


================
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);
----------------
lebedev.ri wrote:
> 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?
> 
Sorry for the misunderstanding. 

I think what you are suggesting is to recreate the misexpect metadata from the MD_prof "branch_weights metadata. Is that right?

I think that may be possible, but it will add complexity. While the metadata approach is very straightforward. I think there is value in keeping this simple, and then iterating on it. 

Also, LowerExpectIntrinsic.cpp has a comment near the top suggesting that the LikelyBranchWeight and UnlikelyBranchWeights be hoisted to a common space. This would significantly shrink the size of the misexpect metadata if it were accessible directly. However, that is a change that should happen separately from this patch, and might require further discussion to find the correct shared space.

Ultimately, however, I do not see a way to completely remove some kind of metadata tag, as we only need to perform misexpect checks when the intrinsic was used -- which should be a small minority of cases. If there is another way, then I am happy to hear it, but again, maybe that should be another patch.





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

https://reviews.llvm.org/D66324