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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/include/llvm/IR/Instruction.h:287-293
+  /// Get the metadata of given kind attached to this Instruction. Requires
+  /// that the instruction has at most a single attachment of the given kind.
   /// If the metadata is not found then return null.
   MDNode *getMetadata(StringRef Kind) const {
     if (!hasMetadata()) return nullptr;
     return getMetadataImpl(Kind);
   }
----------------
tejohnson wrote:
> dexonsmith wrote:
> > What happens if there are multiple? Assert, return first, return arbitrary, or return `nullptr`?
> I addressed this in my comments above too, but to reiterate, this relies on the behavior of the underlying Value::getMetadata support, and that support just seems to return the first matching one, which is a regression in the behavior from the original support added in D20414, which asserted if there was more than one. I need to go back and see where/why that assert was removed before it was refactored into the Value class in D67626. Since Instruction will use the underlying Value support, it should be fixed there.
It turns out the assert was removed in D38184. Looks like this was to move handling of malformed debug info to AutoUpgrade. Per patch:
"Prior to this patch, a NoAsserts build would have printed a
warning and stripped the debug info and an Asserts build would have
asserted, after this patch the Verifier will report a fatal error."

I'm assuming the assert it refers to is the one removed in Metadata.cpp in that patch. Perhaps the assert should be left it but restricted to non-dbg metadata?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105251