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

[David Li via Phabricator via llvm-commits]

davidxl accepted this revision.
davidxl added a comment.

lgtm



================
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:
> 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?
IRPGO scales the count before setting the edge weights.  Frontend based instrumentation does the same.  Static branch weights have small values so they are fine too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141393