[PATCH] D131306: [llvm][misexpect] Track provenance of branch weights

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Sorry for the slow response. Looks pretty good, just a few minor suggestions and questions.



================
Comment at: llvm/include/llvm/IR/MDBuilder.h:61
 
   /// Return metadata containing two branch weights.
+  MDNode *createBranchWeights(uint32_t TrueWeight, uint32_t FalseWeight,
----------------
Update comment to mention new parameter in this and the below methods.

Also, I wonder if it would be better to make the IsExpected non-optional, to force any new callers to think about whether they need that and help avoid issues where it gets dropped incorrectly?


================
Comment at: llvm/include/llvm/IR/ProfDataUtils.h:58
 
+/// Check if Branch Weight Metadata has an "expected" field
+bool hasExpectedProvenance(const Instruction &I);
----------------
Maybe note the meaning of the expected field (i.e. had an llvm.expect intrinsic).


================
Comment at: llvm/lib/Analysis/BranchProbabilityInfo.cpp:394
+  // Ensure there are weights for all of the successors. Note that the first
+  // operand to the metadata node is a name, not a weight.
+  if (WeightsNode->getNumOperands() !=
----------------
Isn't it now the first 1 or 2 are a name?


================
Comment at: llvm/lib/Analysis/BranchProbabilityInfo.cpp:397
+      TI->getNumSuccessors() + getBranchWeightOffset(WeightsNode))
+    return false;
+
----------------
Should this be an assert?


================
Comment at: llvm/lib/IR/Instructions.cpp:732
       ProfileData->getNumOperands() > 0) {
+    auto Offset = getBranchWeightOffset(ProfileData);
     // Using APInt::div may be expensive, but most cases should fit 64 bits.
----------------
I guess we don't have to worry about pushing the expect provenance because this is a call and it shouldn't have one. Maybe add an assert that the offset is 1?


================
Comment at: llvm/lib/IR/Verifier.cpp:4553
   StringRef ProfName = MDS->getString();
+  unsigned Offset = getBranchWeightOffset(I);
 
----------------
Related to an earlier comment regarding calls - maybe the verifier should ensure that "expected" only shows up on certain instruction types?


================
Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:156
+  // weights are being added multiple times, as is the case for SampleProfiling
+  // under ThinLTO. We gate all known entry paths to verifyMisExpect() by first
+  // checking for the presence of the "expected" tag in the metadata, which is
----------------
Why not check it within verifyMisExpect, seems less error prone than requiring callers to check?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131306