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

[Mingming Liu via Phabricator via llvm-commits]

mingmingl marked an inline comment as done.
mingmingl added inline comments.


================
Comment at: llvm/lib/IR/Metadata.cpp:1075
 
+MDNode *MDNode::getMergedProfMetadata(MDNode *A, MDNode *B,
+                                      const Instruction *AInstr,
----------------
davidxl wrote:
> 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.
>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.

This is true. Here either the caller 'combineMetadata' or callee ('getMergedProfMetadata' or 'mergeDirectCallProfMetadata') needs to look at 'AInstr' or 'BInstr' to decide if it's the implemented case, and it looks cleaner (in the caller) by putting details in the callee 'getMergedProfMetadata'

Added a private method 'mergeDirectCallProfMetadata' to make future expansions easier.


================
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");
----------------
davidxl wrote:
> how about == 2?
'== 2' works for 'callinstr' with 'branch_weights' (e.g., after seeing `AProfName.equals("branch_weights") && BProfName.equals("branch_weights")`) -> yet `!prof` for direct callsites might be expanded in the future (like `invoke` prof could have both branch weights and value profiles) so here make assertions closer what LLVM verifier already verifies.


================
Comment at: llvm/lib/IR/Metadata.cpp:1116
+        mdconst::dyn_extract<ConstantInt>(B->getOperand(1));
+    if (AInstrWeight && BInstrWeight) {
+      return MDNode::get(Ctx,
----------------
davidxl wrote:
> why is the check needed?
Changed it to assert since verifier already has `Check(mdconst::dyn_extract<ConstantInt>(MDO),  "!prof brunch_weights operand is not a const int");`


================
Comment at: llvm/test/Transforms/SimplifyCFG/merge-direct-call-branch-weights-in-hoist.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
+; RUN: opt < %s -passes='simplifycfg<no-sink-common-insts;hoist-common-insts>' -simplifycfg-require-and-preserve-domtree=1 -S | FileCheck %s --check-prefix=HOIST
----------------
nikic wrote:
> You need `--check-globals` to check the resulting metadata. Also, it should be possible to reduce these tests to basically an if/else with a call each.
thanks! Reduced the test case and added '-check-globals'


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

https://reviews.llvm.org/D148877