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

[Roman Lebedev via Phabricator via cfe-commits]

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