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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
