[PATCH] D141393: [llvm][ir] Purge MD_prof custom accessors

[Paul Kirth via Phabricator via llvm-commits]

paulkirth added a comment.





================
Comment at: llvm/lib/IR/ProfDataUtils.cpp:104-124
+bool hasValidBranchWeightMD(const Instruction &I) {
+  auto *ProfileData = I.getMetadata(LLVMContext::MD_prof);
+  if (!isBranchWeightMD(ProfileData))
+    return false;
+  if (ProfileData && ProfileData->getNumOperands() == 1 + I.getNumSuccessors())
+    return true;
+  return false;
----------------
I'd consider inverting the logic between `getValidBranchWeightMDNode()` and `hasValidBranchWeightMD()`


================
Comment at: llvm/lib/Transforms/Scalar/LoopPredication.cpp:992
+      uint64_t Numerator = 0, Denominator = 0;
+      for (auto [i, Weight] : llvm::enumerate(Weights)) {
         if (Term->getSuccessor(i) == ExitBlock)
----------------
I always forget the `enumerate` API exists ... what a massive improvement to readability. 


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:225
           unsigned idx = i->getCaseIndex();
+          // TODO: Add overflow check.
           Weights[0] += Weights[idx+1];
----------------
Should we track this improvement w/ an issue? If you don't want to that's fine, but I always lose track of these TODOs unless I file something in a bug tracker and ref it from my code. Totally up to you though.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:315
                                                SI->getDefaultDest());
-      MDNode *MD = SI->getMetadata(LLVMContext::MD_prof);
-      if (MD && MD->getNumOperands() == 3) {
-        ConstantInt *SICase =
-            mdconst::dyn_extract<ConstantInt>(MD->getOperand(2));
-        ConstantInt *SIDef =
-            mdconst::dyn_extract<ConstantInt>(MD->getOperand(1));
-        assert(SICase && SIDef);
+      SmallVector<uint32_t> Weights;
+      if (extractBranchWeights(*SI, Weights) && Weights.size() == 2) {
----------------
`uint64_t`, maybe? I can never remember what is guaranteed to run before we scale down to 32-bit weights, but the size should at least be the same between the weights vector and `DefWeight/CaseWeight`, right? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141393