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

[Roman Lebedev via Phabricator via cfe-commits]

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