[PATCH] D66324: clang-misexpect: Profile Guided Validation of Performance Annotations in LLVM

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

Cool, I look forward to checking for outdated `__builtin_expects` in the Linux kernel, if they exist!



================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:108
+      auto *L = mdconst::dyn_extract<ConstantInt>(MisExpectData->getOperand(2));
+      auto *U = mdconst::dyn_extract<ConstantInt>(MisExpectData->getOperand(3));
+
----------------
What's C, L, and U?


================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:116
+          std::accumulate(Weights.begin(), Weights.end(), (uint64_t)0,
+                          [](uint64_t W1, uint64_t W2) { return W1 + W2; });
+      int NumUnlikelyTargets = Weights.size() - 1;
----------------
can `std::plus` be used here?


================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:122
+
+      double Percentage = ((double)ProfileCount / (double)CaseTotal);
+
----------------
are the extra parens necessary?


================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:142
+    // Operand 0 is a string tag "VP":
+    if (MDString *Tag = cast<MDString>(MD->getOperand(0))) {
+      if (NOps > 1 && Tag->getString().equals("branch_weights")) {
----------------
Do you need to check first that `NOps > 0` first? Otherwise you can likely move the creation of `NOps` below this conditional.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66324/new/

https://reviews.llvm.org/D66324