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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits