lebedev.ri added a comment.
Trying to start reviewing this.
The llvm side of the patch is self contained; clang patch should be split into
a dependent review.
================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:626
+ unsigned Line, Column;
+ bool BadDebugInfo = false;
+ FullSourceLoc Loc =
----------------
This should be `BadDebugInfoLoc` (in `getBestLocationFromDebugLoc()` too)
================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:28-1361
#include "clang/AST/StmtObjC.h"
#include "clang/Basic/Builtins.h"
#include "clang/Basic/CodeGenOptions.h"
#include "clang/Basic/TargetInfo.h"
#include "clang/CodeGen/CGFunctionInfo.h"
#include "clang/Frontend/FrontendDiagnostic.h"
#include "llvm/IR/DataLayout.h"
----------------
Dubious changes, drop
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3451-3454
+ const std::vector<std::string> &warnings = Res.getDiagnosticOpts().Warnings;
+ if (std::find(warnings.begin(), warnings.end(), "misexpect") !=
+ warnings.end())
+ Res.FrontendOpts.LLVMArgs.push_back("-pgo-warn-misexpect");
----------------
Here you are checking that `-Wmisexpect` is passed to clang?
This doesn't seem idiomatic. Was this copied from somewhere?
Normally this is `Diags.isIgnored(diag::warn_profile_data_misexpect, ???)`
================
Comment at: llvm/include/llvm/Transforms/Utils/MisExpect.h:1
+//===--- MisExpect.cpp - Check Use of __builtin_expect() with PGO data
----===//
+//
----------------
`__builtin_expect()` is a clang-specific thing.
Can all instances of it in all comments be reworded to be more agnostic?
================
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);
----------------
Why can't `LLVMContext::MD_prof` be reused?
================
Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:82
- SI.setCondition(ArgValue);
return true;
----------------
Why do you need to move this line?
================
Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:84
+ SI.setCondition(ArgValue);
+ misexpect::checkClangInstrumentation(SI);
----------------
clang is not the only llvm frontend, this should not be so specific
================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:30
+#include "llvm/Support/FormatVariadic.h"
+#include <bits/stdint-uintn.h>
+#include <functional>
----------------
This doesn't look correct.
================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:33
+#include <numeric>
+#include <stdint.h>
+
----------------
<cstdint>
================
Comment at: llvm/test/Transforms/LowerExpectIntrinsic/basic.ll:288-289
; CHECK: !0 = !{!"branch_weights", i32 2000, i32 1}
-; CHECK: !1 = !{!"branch_weights", i32 1, i32 2000}
-; CHECK: !2 = !{!"branch_weights", i32 1, i32 2000, i32 1}
-; CHECK: !3 = !{!"branch_weights", i32 2000, i32 1, i32 1}
+; CHECK: !2 = !{!"branch_weights", i32 1, i32 2000}
+; CHECK: !4 = !{!"branch_weights", i32 1, i32 2000, i32 1}
----------------
`; CHECK: !1 = ???` ?
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
