[PATCH] D65300: [clang] [CodeGen] clang-misexpect prototype for compiler warnings

[Petr Hosek via Phabricator via cfe-commits]

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