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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D105251#2856025 <https://reviews.llvm.org/D105251#2856025>, @dexonsmith wrote:

> In D105251#2855987 <https://reviews.llvm.org/D105251#2855987>, @tejohnson wrote:
>
>> 1. My primary motivation: allow multiple new heap prof metadatas on a single allocation call without indirection, via multiple !prof attachments each with the same tag. E.g.
>>
>>   call malloc !prof !0, !prof !1
>>   
>>   !0 = !{"heap", [stack trace 1 heap profile data]}
>>   !1 = !{"heap", [stack trace 2 heap profile data]} 
>
> Ah, I'd completely missed this, I was just thinking about the orthogonal metadata case.
>
> Seems like a separate `!heapprof` attachment that uses the feature from this patch (leaving `!prof` alone, limited by the verifier to 1-per-instruction) could be pretty clean, but as you say, there are a number of reasonable options.
>
>> Agreed. Let me discuss this with my collaborators and think about it a little more. But yes, in either case let's plan to update LangRef. I was surprised that it didn't mention anything in this regard. Will circle back probably after the holiday.
>
> Sounds great!

I think we will go in the direction of using new metadata attachment kinds, with indirection, to avoid this issue completely. I can write up and send you a langref patch, unless you think this warrants an llvm-dev discussion first?

In D105251#2873086 <https://reviews.llvm.org/D105251#2873086>, @wenlei wrote:

> 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.

This is a very interesting idea. We can consider using a metadata format for the context portion of the profile that can be eventually shared with other types of profile attachments. BTW we will be sending an RFC soon-ish for the binary profile and metadata formats.

> 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"..

In some ways, although the VP profile is quite a bit simpler IMO (just a list of ints with hardcoded meaning, whereas the heap profile will have a variety of profiling statistics) and probably deserves to stay separate from the heap profiling. But yes, it is possible it should be separated from the branch weights.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105251