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

[Paul Kirth via Phabricator via llvm-commits]

paulkirth accepted this revision.
paulkirth added a comment.
This revision is now accepted and ready to land.

This is LGTM from my perspective, as long as the 32-bit usage in Local.cpp is safe. Thanks for taking this up. I think it's a big improvement.



================
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) {
----------------
Dinistro wrote:
> 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.
I think this is fine but we may want to double check that this only gets called after scaling down to 32-bit. Otherwise, maybe we should have a uint64_t version too, or make this a templated function in the .h, so the you can choose at the callsite. That can be a separate change though, as long as 32-bit is safe.

@davidxl do you know if this gets called after scaling down to 32-bit?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141393