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

[Paul Kirth via Phabricator via llvm-commits]

paulkirth marked an inline comment as done.
paulkirth added a comment.

Thanks for the review.

I'm working on splitting up the patch now.

I should be able to address most of your comments, but I left some inline comments for clarification/discussion.



================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:626
+  unsigned Line, Column;
+  bool BadDebugInfo = false;
+  FullSourceLoc Loc =
----------------
lebedev.ri wrote:
> This should be `BadDebugInfoLoc` (in `getBestLocationFromDebugLoc()` too)
That should be in a separate patch, right?


================
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:
> 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


================
Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:82
 
-  SI.setCondition(ArgValue);
   return true;
----------------
lebedev.ri wrote:
> Why do you need to move this line?
I did this so that the condition is available to check when performing validation, and the metadata won't be replaced by the likely/unlikely values until after I've finished the validation. Having the condition available in when performing the check provides for better diagnostics output.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66324