This is an archive of the discontinued LLVM Phabricator instance.

IR: Allow multiple global metadata attachments with the same type.

Authored by pcc on May 18 2016, 10:41 PM.



This will be necessary to allow the global merge pass to attach
multiple debug info metdata nodes to global variables once we reverse
the edge from DIGlobalVariable to GlobalVariable.

Diff Detail


Event Timeline

pcc updated this revision to Diff 57736.May 18 2016, 10:41 PM
pcc retitled this revision from to IR: Allow multiple global metadata attachments with the same type..
pcc updated this object.
pcc added reviewers: aprantl, dexonsmith, dblaikie.
pcc added a subscriber: llvm-commits.
aprantl edited edge metadata.May 20 2016, 9:26 AM

Couple of small inline comments.

108 ↗(On Diff #57736)

Short comment?

1159 ↗(On Diff #57736)

Why is this check necessary?

1327 ↗(On Diff #57736)


if (hasMetadata())
  getContext().pImpl->GlobalObjectMetadata[this].get(KindID, MDs);
1334 ↗(On Diff #57736)

Same here

1338 ↗(On Diff #57736)
if (!MD)
1343 ↗(On Diff #57736)


1382 ↗(On Diff #57736)

This condition is redundant since addMetadata will check for a nullptr.

2002 ↗(On Diff #57736)
++NumDebugAttachments <= 1

can never be <1, right?

pcc updated this revision to Diff 57974.May 20 2016, 1:03 PM
pcc marked 4 inline comments as done.
pcc edited edge metadata.
  • Address review comments
1159 ↗(On Diff #57736)

It isn't, removed.

1338 ↗(On Diff #57736)

Rethinking this, maybe this function should always expect an non-null MDNode, since unlike setMetadata the intent is always to add an attachment. WDYT?

1382 ↗(On Diff #57736)

See above

2002 ↗(On Diff #57736)

No, changed to ==

aprantl added inline comments.May 31 2016, 11:33 AM
1013 ↗(On Diff #57974)

Comment mentioning what happens if the ID is invalid?

1014 ↗(On Diff #57974)

No t sure: should we define a type that inherits from std:pair<unsigned, MDNode &> and defines an operator< or would that be over-engineering?

1014 ↗(On Diff #57974)

Usually, we're calling this method insert().

1016 ↗(On Diff #57974)

Comment explaining the sorting guarantees on the output?

1332–1333 ↗(On Diff #57974)

Could/should we then change the signature to use MDNode &MD?

1385 ↗(On Diff #57974)

The doxygen comment in the header should probably mention that this only works for singular attachments, though it's kind of obvious from the function signature.

1388 ↗(On Diff #57974)

Please add an assertion message.

2002 ↗(On Diff #57974)

Thanks! I also think I'd be more comfortable with the hoisting the increment onto its own line, given that AssertDI is a macro and all.

2244 ↗(On Diff #57974)

Can you remind me why it is safe to drop this test without replacement?

pcc updated this revision to Diff 59162.May 31 2016, 5:49 PM
pcc marked 7 inline comments as done.
  • Address review comments
1014 ↗(On Diff #57974)

To me it would be clearer to define the ordering just in getAll. Defining the ordering in an operator< function would imply that two attachments with the same ID are "equal", while at the moment it's clear from reading the code for getAll that that isn't the case.

1385 ↗(On Diff #57974)

From GlobalObject.h:

The functions that return an MDNode require that the function have at most a
single attachment of the given kind


But I've split up the doxygen comment for getMetadata into two "blocks" so it's clearer which comments pertain to which functions.

2244 ↗(On Diff #57974)

Its purpose is to test the GlobalObject::dropUnknownMetadata function, which has no non-test users [1], so there's no point in keeping it.

aprantl accepted this revision.May 31 2016, 5:59 PM
aprantl edited edge metadata.

Looking good now.

1014 ↗(On Diff #59162)


This revision is now accepted and ready to land.May 31 2016, 5:59 PM
This revision was automatically updated to reflect the committed changes.