[PATCH] D148877: [PGO]Implement metadata combine for 'branch_weights' of direct callsites when none of the instructions folds the rest away

[Mingming Liu via Phabricator via llvm-commits]

mingmingl added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:2721
+      case LLVMContext::MD_prof:
+        K->setMetadata(Kind, MDNode::getMergedProfMetadata(KMD, JMD, K, J));
+        break;
----------------
nikic wrote:
> mingmingl wrote:
> > davidxl wrote:
> > > nikic wrote:
> > > > This should probably be behind a DoesKMove check? If we're CSEing with a dominating call, it doesn't make sense to add the branch weights, as the second call is being removed.
> > > > 
> > > > I'd suggest adding a test with a readonly/nounwind/willreturn call being GVNed to cover that case.
> > > good catch.
> > thanks for the catch!
> > 
> > Just to confirm I'm getting the point (not particular to a specific pass yet), in https://gcc.godbolt.org/z/nGa6YqWdP, two calls to 'func1' in line 11 and line14 should keep one branch weight (rather than the sum of two). So that is the case when unifying (not merge) is correct.
> Yeah, that's the case I had in mind. After the transform only the first call will be executed, so it would make sense to me to only keep its call count.
https://gcc.godbolt.org/z/9cnd3qEce is a test case when EarlyCSE CSE'ed call to 'func1', while turns out the current implementation doesn't invoke merge-metadata around [[ https://github.com/llvm/llvm-project/blob/d94fe9728087f905dbd8fc32bc5c4a096b4d28c5/llvm/lib/Transforms/Scalar/EarlyCSE.cpp#L1413 | where ]] calls are CSE'ed (probably goes to a follow-up patch)

Modified existing test case in GVN.


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

https://reviews.llvm.org/D148877