[PATCH] D105251: [IR] Allow multiple instruction metadata attachments with same type

Mon Jul 12 22:30:05 PDT 2021

wenlei added a comment.

Allowing multiple md_prof in general and the set, update vs add problem @dexonsmith pointed out reminds me of the bug D69736 <https://reviews.llvm.org/D69736> was fixing. So I can see that concern too.

Not trying to muddy the discussion as the comments below doesn't directly help the situation with allowing multiple md_prof. But ...

1. I'm wondering if we should consider design the profile metadata in a way that context info can be (optionally) extended to all profile info too. E.g. branch_weights can have optional context info too, and it's backward compatible in the sense that omitting context info naturally falls back to today's context-less profile. And heap profile just uses the same metadata representation/structure except that it always have the optional context info filled. Saying that because with CSSPGO, we do have context for all branch weights stuff, though currently all such context will be lost if corresponding inlining doesn't happen when loading the profile. If we do that, we'd need to make sure there's no overhead when the optional context isn't present though.

2. Without the general profile context support above (which may not worth the complexity if the overhead can't be zero), the heap profile metadata feels closer to "VP" rather than "branch_weights" - "VP" tag consists of a list of value/count pair, which could be generalize into key-value pairs that covers both today's VP and heap profile (key being context, and value being the heap profile). This is related to the comment on "split out !valueprof at some point to make it orthogonal from branch weights"..

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105251