[PATCH] D133121: [PGO] Annotate branch_weight for invoke instruction

[Rong Xu via Phabricator via llvm-commits]

xur added inline comments.


================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:1077
+  InvokeVals.push_back(MDNode::get(Ctx, Vals));
+  MDNode *NewM = MDNode::getDistinct(Ctx, InvokeVals);
+  NewM->replaceOperandWith(0, NewM);
----------------
hoy wrote:
> Can we use `get` instead of  `getDistinct` so that  invokes with same metadata can potentially share the same node?
Yes. get() seems to be better suited here.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:2168
+  ProfileData = InvokeInst::getIndirectCallProfData(ProfileData);
+  if (!ProfileData) {
+    uint64_t TotalWeight;
----------------
hoy wrote:
> Should the check be `if (ProfileData)`?  If the original invoke doesn't have !VP, the new call should end up with no metadata based on the existing behavior.
If the original invoke does not have VP, it can still have branch_weights.

Maybe my words in patch description is confusing: this patch tries to fix the case where invoke calls to a indirect function.
If the argument of invoke is a direct function -- it will only have a branch_weight and we are doing the right annotations.

For this transformation, mostly like the invoke is a direct function, in which case, we will annotate a total counts -- FDO will not use
this metadata. AutoFDO might use this in the inliner.

If here the newcall is an indirect function, we have have two annotations in invoke and can only have one in the new call. I choose to keep the indirect metadata. This changes the semantic but I think it's the right thing to do.


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

https://reviews.llvm.org/D133121