lebedev.ri accepted this revision. lebedev.ri marked an inline comment as done. lebedev.ri added a comment.
Okay, thank you for the patience, this looks good to me now too. Please update patch description/subject - this patch is not a standalone tool. ================ Comment at: clang/test/Profile/misexpect-branch-cold.c:4 +// RUN: llvm-profdata merge %S/Inputs/misexpect-branch.proftext -o %t.profdata +// RUN: %clang_cc1 %s -O2 -o - -disable-llvm-passes -emit-llvm -fprofile-instrument-use-path=%t.profdata -verify -Wmisexpect + ---------------- Is there a test where `-Wmisexpect` isn't present, to verify that it is off-by-default? ================ 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: > > 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. > > > > nonnull Yes. > ... Well, alright i guess, please add a note that this was at least considered into the patch description. ================ 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 ---------------- paulkirth wrote: > lebedev.ri wrote: > > Here and elsewhere: > > is `!prof !1` no longer present? > The misexpect metadata tags take those slots. > > `br i1 %tobool, label %if.then, label %if.end, !prof !0, !misexpect !1` > > Unless my understanding of how the tags work is flawed, I think this is the > expected behavior I was worried that here the `!prof` were no longer emitted, but only `!misexpect` were emitted. This seems not to be the case, so all good i think. 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
