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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
