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

[Paul Kirth via Phabricator via cfe-commits]

paulkirth added inline comments.


================
Comment at: llvm/include/llvm/IR/MDBuilder.h:61
 
   /// Return metadata containing two branch weights.
+  MDNode *createBranchWeights(uint32_t TrueWeight, uint32_t FalseWeight,
----------------
tejohnson wrote:
> 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?
> 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?

I think that's a good idea. I can see how much that affects this patch after addressing the current set of comments.



================
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() !=
----------------
tejohnson wrote:
> Isn't it now the first 1 or 2 are a name?
Actually, this code block doesn' need to be here at all anymore. That check was moved up into `getValidBranchWeightMDNode`. I must  have missed that when I rebased.


================
Comment at: llvm/lib/Analysis/BranchProbabilityInfo.cpp:397
+      TI->getNumSuccessors() + getBranchWeightOffset(WeightsNode))
+    return false;
+
----------------
tejohnson wrote:
> Should this be an assert?
Same as above. This wasn't an assert originally, so I didn't change its behavior.


================
Comment at: llvm/lib/IR/Verifier.cpp:4553
   StringRef ProfName = MDS->getString();
+  unsigned Offset = getBranchWeightOffset(I);
 
----------------
tejohnson wrote:
> Related to an earlier comment regarding calls - maybe the verifier should ensure that "expected" only shows up on certain instruction types?
I think the issue is that we've overload "branch_weights" for lots of things that aren't branch weights, but are similar.  IMO it should be something like: `!{!"call_count", i32 2000}`, but that likely would break a number of things in the codebase, and require lots of special handling for the various different uses. Right now the IR change I've made doesn't really disambiguate the various cases and just treats them all uniformly.

I'll take a look at updating the verifier rules after addressing the other comments.


================
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
----------------
tejohnson wrote:
> Why not check it within verifyMisExpect, seems less error prone than requiring callers to check?
Centralizing the checking would be ideal, but because Frontend and Backend checking are not completely symmetric, we cannot rely on the instruction having the `Expected` flag set in the metadata when `verifyMisExpect` is called for Frontend based instrumentation. 

Maybe I should update the comment to reflect that the "provenance" check ensures we won't run backend checks multiple times, and put a more explicit comment about it in `checkBackendInstrumentation`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131306