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

[Christian Ulmann via Phabricator via llvm-commits]

Dinistro added inline comments.


================
Comment at: llvm/lib/IR/Instructions.cpp:4606-4609
-  if (ProfileData->getNumOperands() != SI.getNumSuccessors() + 1) {
-    llvm_unreachable("number of prof branch_weights metadata operands does "
-                     "not correspond to number of succesors");
-  }
----------------
paulkirth wrote:
> I don't think we do this check in ProfDatUtils, so we probably shouldn't remove it, should we? I know I've tripped this assert before when modifying some passes, so I think it's good one to keep.
I changed this back to have the `llvm_unreachable`. The newly added `getValidBranchWeightMDNode` could be used here but that would not cause an assertion violation.


================
Comment at: llvm/lib/IR/ProfDataUtils.cpp:46
 // the minimum number of operands for MD_prof nodes with branch weights
-constexpr unsigned MinBWOps = 3;
+constexpr unsigned MinBWOps = 2;
 
----------------
davidxl wrote:
> paulkirth wrote:
> > This constant is used to check the validity of branch weight metadata. Changing this to 2  implies that a MD_prof node w/ only the "branch_weight" moniker and a single weight are valid. I don't think that's true, so I don't think this should change unless we change how MD_prof is laid out as a whole.
> agree. What the reason to change it to 2?
I changed this because calls can also have branch weights: https://llvm.org/docs/BranchWeightMetadata.html#callinst
It turns out that branch weights of calls different from other branch weights and can thus either way not be handled with the current state of this utility, so I reverted this part.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:225
           unsigned idx = i->getCaseIndex();
+          // TODO: Add overflow check.
           Weights[0] += Weights[idx+1];
----------------
paulkirth wrote:
> 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.
Good idea, I tracked this and another problem with the subsequent swap in here: https://github.com/llvm/llvm-project/issues/59956


================
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) {
----------------
paulkirth wrote:
> `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? 
The `extractBranchWeights` function expects an `SmallVectorImp<uint32_t>`, so I now changed the `Weight` variables to `uint32_t` as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141393