lebedev.ri added a comment.
Missing tests for `__builtin_unpredictable()`.
================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:626
+ unsigned Line, Column;
+ bool BadDebugInfo = false;
+ FullSourceLoc Loc =
----------------
paulkirth wrote:
> lebedev.ri wrote:
> > This should be `BadDebugInfoLoc` (in `getBestLocationFromDebugLoc()` too)
> That should be in a separate patch, right?
Yeah, let's keep that for later.
================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:369
void UnsupportedDiagHandler(const llvm::DiagnosticInfoUnsupported &D);
+ void MisExpectDiagHandler(const llvm::DiagnosticInfoMisExpect &D);
/// Specialized handlers for optimization remarks.
----------------
Comment missing
================
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");
----------------
Err, this works? I'd expect `!` to be missing here..
================
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:
> > 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?
================
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
----------------
Here and elsewhere:
is `!prof !1` no longer present?
================
Comment at: llvm/test/Transforms/LowerExpectIntrinsic/basic.ll:292-293
+; CHECK: !3 = !{!"misexpect", i64 1, i64 2000, i64 1}
+; CHECK: !4 = !{!"branch_weights", i32 1, i32 2000, i32 1}
+; CHECK: !5 = !{!"branch_weights", i32 2000, i32 1, i32 1}
----------------
Should there be a FIXME, are some `misexpect` missing here?
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
