Page MenuHomePhabricator

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

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

Details

Summary

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.

include/llvm/IR/GlobalObject.h
108

Short comment?

lib/IR/Metadata.cpp
1159

Why is this check necessary?

1327

Maybe:

if (hasMetadata())
  getContext().pImpl->GlobalObjectMetadata[this].get(KindID, MDs);
1334

Same here

1338
if (!MD)
  return;
1343

redundant

1382

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

lib/IR/Verifier.cpp
2002
++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
lib/IR/Metadata.cpp
1159

It isn't, removed.

1338

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

See above

lib/IR/Verifier.cpp
2002

No, changed to ==

aprantl added inline comments.May 31 2016, 11:33 AM
lib/IR/LLVMContextImpl.h
1013

Comment mentioning what happens if the ID is invalid?

1014

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

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

1016

Comment explaining the sorting guarantees on the output?

lib/IR/Metadata.cpp
1336–1340

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

1390–1396

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.

1393

Please add an assertion message.

lib/IR/Verifier.cpp
2002

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.

unittests/IR/MetadataTest.cpp
2244–2245

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
lib/IR/LLVMContextImpl.h
1014

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.

lib/IR/Metadata.cpp
1390–1396

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.

unittests/IR/MetadataTest.cpp
2244–2245

Its purpose is to test the GlobalObject::dropUnknownMetadata function, which has no non-test users [1], so there's no point in keeping it.
[1] http://llvm-cs.pcc.me.uk/lib/IR/Metadata.cpp/rdropUnknownMetadata

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

Looking good now.

lib/IR/LLVMContextImpl.h
1014

Ok.

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.