[PATCH] D148877: [PGO]Implement metadata combine for 'branch_weights' of direct callsites

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/include/llvm/IR/Metadata.h:1322
   static MDNode *getMostGenericAlignmentOrDereferenceable(MDNode *A, MDNode *B);
+  /// Returns nullptr if merge is not implementated for the given types
+  /// instructions and !prof metadata. Currently only implemented with direct
----------------
Have an one line summary of what the interface does here. Move the detailed documentation to the function definition.


================
Comment at: llvm/lib/IR/Metadata.cpp:1075
 
+MDNode *MDNode::getMergedProfMetadata(MDNode *A, MDNode *B,
+                                      const Instruction *AInstr,
----------------
Why not naming it 'mergeDirectCallProfMetadata'?  Even if there is a need to merge indirect call profiles, the implementation will be quite different and may require a different method for it.


================
Comment at: llvm/lib/IR/Metadata.cpp:1078
+                                      const Instruction *BInstr) {
+  if (!A)
+    return B;
----------------
Combine these two into if (! (A&&B)) return;


================
Comment at: llvm/lib/IR/Metadata.cpp:1101
+  // LLVM IR verifier verifies !prof metadata has at least 2 operands.
+  assert(A->getNumOperands() >= 2 && B->getNumOperands() >= 2 &&
+         "!prof annotations should have no less than 2 operands");
----------------
how about == 2?


================
Comment at: llvm/lib/IR/Metadata.cpp:1116
+        mdconst::dyn_extract<ConstantInt>(B->getOperand(1));
+    if (AInstrWeight && BInstrWeight) {
+      return MDNode::get(Ctx,
----------------
why is the check needed?


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

https://reviews.llvm.org/D148877