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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

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

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

> 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