[PATCH] D80618: Extend InvokeInst !prof branch_weights metadata to unwind branches

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Mostly minor comments on style to help drive review forward.  This is a drive by comment, don't wait on me please.

I generally think this patch is a really good idea.  Longer term, I think we should extend this to calls as well.  There's nothing preventing a call from being hot throw w/o a local exception handler and the current static heuristic doesn't consider that case.  I specifically do not want this rolled into the current patch.  That would be scope creep.  :)



================
Comment at: llvm/docs/BranchWeightMetadata.rst:98
+    metadata !"branch_weights",
+    i32 <INVOKE_BRANCH_WEIGHT>
+    [ , i32 <INVOKE_UNWIND_WEIGHT> ]
----------------
I'd suggest calling this INVOKE_NORMAL_WEIGHT.  


================
Comment at: llvm/lib/IR/Instruction.cpp:781
          "Can only set weights for call like instructions");
-  SmallVector<uint32_t, 1> Weights;
-  Weights.push_back(W);
-  MDBuilder MDB(getContext());
-  setMetadata(LLVMContext::MD_prof, MDB.createBranchWeights(Weights));
+  assert(isa<CallBase>(&SrcCB) &&
+         "Can only set weights for call like instructions");
----------------
Why not just have the argument be of CallBase type then?


================
Comment at: llvm/lib/IR/Instruction.cpp:791
+
+  if (!ProfDataName->getString().equals("branch_weights") &&
+      !(ProfDataName->getString().equals("VP") &&
----------------
Note sure you need these validation checks here since the source can be presumed valid.  


================
Comment at: llvm/lib/IR/Verifier.cpp:4154
   if (ProfName.equals("branch_weights")) {
     unsigned ExpectedNumOperands = 0;
     if (BranchInst *BI = dyn_cast<BranchInst>(&I))
----------------
Please rearrange this code as:
if (is invoke) {
  do invoke check;
} else {
  do current code block;
}


================
Comment at: llvm/lib/Transforms/IPO/ArgumentPromotion.cpp:340
     NewCS->setDebugLoc(CB.getDebugLoc());
-    uint64_t W;
-    if (CB.extractProfTotalWeight(W))
-      NewCS->setProfWeight(W);
+    NewCS->copyProfMetadata(CB);
     Args.clear();
----------------
It looks like copyProfMetadata could be pulled out into a separate NFC.  Please do so.


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

https://reviews.llvm.org/D80618