[PATCH] D131136: [misexpect] Support diagnostics from frontend profile data

Teresa Johnson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sat Mar 4 07:49:39 PST 2023


tejohnson added a comment.

I'm not sure of the status of these patches, since the base one is still marked "WIP". Some comments below on this patch, but are they all ready for review? If so, WIP should probably be removed from D129889 <https://reviews.llvm.org/D129889>.



================
Comment at: llvm/include/llvm/Target/TargetOptions.h:366
     unsigned MisExpect : 1;
+    unsigned MissingAnnotations : 1;
 
----------------
Needs its own comment.


================
Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:366
+  auto ret = handleBrSelExpect<BranchInst>(BI);
+  if (!ret) {
+    SmallVector<uint32_t> Weights;
----------------
I take it a return value of false from then handle*Expect functions means there was not an expect? Would be good to note this in a comment (here and in the switch handling later). Also, suggest moving this down into lowerExpectIntrinsic to mirror where/how switch are handled.


================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:311
     // to do the check on branches that don't have expect intrinsics
+    verifyMissingAnnotations(I, ExistingWeights);
 
----------------
It doesn't seem like this patch includes a test for this new invocation of verifyMissingAnnotations - can one be added?


================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:317
     // auto RealWeights = RealWeightsOpt.getValue();
     // verifyMissingAnnotations(I, RealWeights, ExistingWeights);
   } else {
----------------
Should these commented out lines be removed now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131136



More information about the llvm-commits mailing list