phosek added inline comments.
================
Comment at: clang/lib/CodeGen/MisExpect.cpp:29
+
+void DebugPrintMisExpectSwitchInfo(SmallVector<uint64_t, 16> *SwitchWeights,
+ llvm::DenseMap<int64_t, size_t> *CaseMap);
----------------
It seems like `DebugPrintMisExpectSwitchInfo` and `EmitMisExpectWarning` is
only used within this file, so I'd move them to anonymous namespace.
================
Comment at: clang/lib/CodeGen/MisExpect.cpp:74
+ LLVM_DEBUG(llvm::dbgs() << "Expected Value:\t" << ExpectedVal << "\n");
+ LLVM_DEBUG(llvm::dbgs() << "Current Count: \t" << CurrProfCount << "\n");
+ LLVM_DEBUG(llvm::dbgs() << "True Count: \t" << TrueCount << "\n");
----------------
No space between `:` and `\t` here and below.
================
Comment at: clang/lib/CodeGen/MisExpect.cpp:82
+ if (ExpectedTrueBranch) {
+ const llvm::BranchProbability HotFunctionThreshold(1, 100);
+ Scaled = HotFunctionThreshold.scale(CurrProfCount);
----------------
Are these thresholds defined anywhere as constants?
================
Comment at: clang/lib/CodeGen/MisExpect.cpp:94
+
+ if (IncorrectPerfCounters) {
+ EmitMisExpectWarning(Call, CGM);
----------------
No need for `{` and `}`.
================
Comment at: clang/lib/CodeGen/MisExpect.cpp:122
+ uint64_t Index;
+ if (MapIndex != CaseMap->end()) {
+ Index = MapIndex->second;
----------------
No need for { and }, in this case you can probably just use ternary expression.
================
Comment at: clang/lib/CodeGen/MisExpect.cpp:133
+
+ if (TakenCount < Max) {
+ EmitMisExpectWarning(Call, CGM);
----------------
No need for { and }.
================
Comment at: clang/lib/CodeGen/MisExpect.h:10
+// This contains code to emit warnings for potentially incorrect usage of
+// __builtin_expect(). It uses PGO profiles to validation.
+//
----------------
`for validation` or `to validate`?
================
Comment at: clang/lib/CodeGen/MisExpect.h:34
+/// CheckMisExpectBranch - check if a branch is annotated with
+/// __builting_expect and when using profiling data, verify that the profile
+/// agrees with the use of the annotation
----------------
s/__builting_expect/__builtin_expect/
================
Comment at: clang/lib/CodeGen/MisExpect.h:36
+/// agrees with the use of the annotation
+
+void CheckMisExpectBranch(const Expr *Cond, uint64_t TrueCount,
----------------
extra empty line
================
Comment at: clang/lib/CodeGen/MisExpect.h:40
+
+/// CheckMisExpect - check if a branch is annotated with __builting_expect and
+/// when using profiling data, verify that the profile agrees with the use of
----------------
s/__builting_expect/__builtin_expect/
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65300/new/
https://reviews.llvm.org/D65300
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
