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