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

I'm a bit concerned about the subtle difference in behaviour... in the past, metadata schemes that needed multiple attachments added a level of indirection (point at a tuple of metadata). Will that work for the heap profiler?

> because it is an inconsistency in IR support
> (the LLVM langref does not specify anything with regards to this
> behavior).

While the current behaviour (one attachment per kind) is not documented in LangRef, it's documented in the header docs, and I do think it was intentional; and in any case, it has been around for a long time and there could be code patterns relying on it.

Since this changes IR semantics, I suggest adding a change to LangRef and a discussion on llvm-dev (and/or the new proposals system).

A few design points that I'm not sure about:

- What should `getMetadata` do if there are multiple attachments for the requested kind? (assert? return nullptr? return the first, last, or arbitrary?)
- Should there be a way for metadata kinds to explicitly opt-in (or out) of allowing multiple attachments? (Maybe could be stored on the LLVMContext somehow?)
  - If not, what do we do about the inconsistency with `!dbg`? Support multiple attachments somehow?
- (And: why is this better than using a level of indirection when multiple attachments are needed?)

I also have a bunch of comments inline assuming this goes forward.



================
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);
   }
----------------
What happens if there are multiple? Assert, return first, return arbitrary, or return `nullptr`?


================
Comment at: llvm/include/llvm/IR/Instruction.h:329
 
+  /// Add a metadata attachment of the specified kind.
+  void addMetadata(unsigned KindID, MDNode &MD);
----------------
Might be good to mention that if `KindID` already exists, this breaks future calls to `getMetadata()`.


================
Comment at: llvm/lib/IR/Metadata.cpp:1358-1361
+  if (KindID == LLVMContext::MD_dbg) {
+    DbgLoc = DebugLoc(&MD);
+    return;
+  }
----------------
Looks like `!dbg` doesn't support multiple instances. Should that be documented? Should this assert instead of silently overriding the existing one? Should the LLParser catch this as an error, or potentially use `setMetadata()` still for `!dbg`?


================
Comment at: llvm/lib/Transforms/Utils/ValueMapper.cpp:924-926
+  I->clearMetadata();
+  for (const auto &MI : MDs)
+    I->addMetadata(MI.first, *cast<MDNode>(mapMetadata(MI.second)));
----------------
- I think this drops handling of `mapMetadata()` returning nullptr (which can happen if the ValueMapper client has mapped some metadata node explicitly). Is that intentional?

- I wonder if there should be a `setAllMetadata()` API matching `getAllMetadata()` that combines the clear and the add loop (but without all the separate densemap lookups). This callsite would still need a loop in the middle to call `mapMetadata()`.


================
Comment at: llvm/test/Bitcode/branch-weight.ll:19
 
 ; RUN: llvm-dis %S/Inputs/branch-weight.bc -o - | FileCheck %s
+; CHECK: !prof !0, !prof !1, !prof !2
----------------
Seems a shame to modify a binary file. Could you instead add a separate test that round-trips to bitcode?


================
Comment at: llvm/test/Bitcode/branch-weight.ll:20-23
+; CHECK: !prof !0, !prof !1, !prof !2
+; We should no longer have the second branch_weights metadata
+; CHECK: !prof !3, !prof !4{{$}}
+; CHECK: !0 = !{!"foo1"}
----------------
I think it'd be good for this to use string substitution blocks rather than relying on stable numbering: https://www.llvm.org/docs/CommandGuide/FileCheck.html#filecheck-string-substitution-blocks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105251