[PATCH] D128858: [llvm] Provide utility function for MD_prof

[Paul Kirth via Phabricator via llvm-commits]

paulkirth added inline comments.


================
Comment at: llvm/include/llvm/IR/ProfDataUtils.h:46
+/// \return false if no metadata was found.
+bool extractTotalBranchWeights(const MDNode *ProfileData,
+                               uint64_t &TotalWeights);
----------------
bogner wrote:
> Would "extractRawBranchWeights" be a better name here? I guess this matches the naming of the user, but I think it's a bit hard to tell the difference between this and the extractBranchWeights functions from the name/doc comments as is.
Maybe we should leave this one as is, and rename the others to `extractRawBranchWeights`? This one seems really clear to me that its extracting the total of all weights. 

One wrinkle here is that to me `extractRawBranchWeights`  communicates more that we're extracting a series of weights in their raw form. Under that reading I'd expect it to be a `SmallVector<ConstantInt*>`, i.e., the raw format in the metadata.

In the meantime I'm going to look around the codebase a bit and see if I can either find a pattern we can use or think of another more communicative name.


================
Comment at: llvm/lib/IR/ProfDataUtils.cpp:50
+      Weights.clear();
+      return false;
+    }
----------------
bogner wrote:
> Can we actually get here if we have valid metadata? This looks like something is malformed
That may be true but most of the places where we're extracting branch weights have similar checks. 

Two examples are:
https://github.com/llvm/llvm-project/blob/3a2d7d8ad5291102ad98b8269b9c2171d3d58d76/llvm/lib/Analysis/BranchProbabilityInfo.cpp#L404
https://github.com/llvm/llvm-project/blob/3a2d7d8ad5291102ad98b8269b9c2171d3d58d76/llvm/lib/IR/Metadata.cpp#L1510

It looks like we didn't follow that pattern in Transform/Utils/MisExpect.cpp, but that may be a mistake on my part.

I can seen an argument in both cases, and I'm not sure one is better than the other. Since I don't have a strong opinion, I'm happy to replace the check with an assert though if you think that's the better choice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128858