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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

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

> In D105251#2854296 <https://reviews.llvm.org/D105251#2854296>, @tejohnson wrote:
>
>> Can you clarify how this change would affect currently working code, since we don't currently support >1 attachment?
>
> With the right design, it probably won't... and we can get the right verifier checks in place to ensure newly-expressible-but-invalid IR doesn't slip through. (Note that the design implied by the header docs, where `getMetadata()` "fails", an existing IR consumer that calls `getMetadata()` has a latent bug since it's going to start crashing if/when an IR producer starts adding multiple attachments. The bug would not be exposed immediately, only once some producer starts using the feature.)
>
> A couple more points I just thought of for `!prof` (which is the kind that would support multiple attachments):
>
> - We probably need verifier that there aren't two competing (e.g.) `!"branch_weights"` entries on the same instruction

Ditto for value profile !prof metadata ("VP" tag).

> - We probably need an API for "setting" the `!"branch_weights"` entry (since blindly calling `addMetadata()` would make the above verifier fail)

Not sure I follow - right now there shouldn't be multiple branch weights prof metadata and I don't see a need for that going forward (the heap profiler data would use a different/new tag for its !prof metadata).

> Also: I thought of another alternative approach to the problem you're trying to solve with `!prof`: move some of the tags to a different/new MDKind. (And if you're introducing a new tag for this feature, you don't even have to update testcases or do a bitcode upgrade, just add another kind to the enum...)

Do you mean add something new like !heapprof? I thought about that, but it seemed cleaner to put all profile data under !prof. But it would solve part of the issue (needing multiple !prof due to other types of profile metadata on the inst), and avoid the need for this change if we also use a level of indirection when there is more than one heap profile per allocation.

>> Ok. Right, the LLParser will silently drop all but the last if you try to use multiple on an instruction with the same type. But the Value implementation returns the first afaict.
>
> IMO the first is a better answer; if no one brings up concrete concerns with that it seems fine to me.
>
>> Thanks for the background and the suggestions below. I think changing that should be orthogonal to adding this general support to instructions though, especially if we go with the allowlist so !dbg only allows one. Or is there another reason we would need to change that first?
>
> Probably not a hard requirement; just seemed cleaner to me to remove the special case for `!dbg` than to add more of them.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105251