This is an archive of the discontinued LLVM Phabricator instance.

[Metadata] Add a resize capability to MDNodes and add a push_back interface to MDNodes.
ClosedPublic

Authored by wolfgangp on May 19 2022, 11:32 AM.

Details

Summary

See the discussion in D124548.

This patch depends on D125994 (the copy/move constructors and assignment ops for MDOperand).
Some notes:

  • I have a slight preference for the term "resizable" over "dynamic", but I'd be happy to change it if you feel strongly about it.
  • I only introduced a push_back() instead of append() for now. Could be introduced incrementally.
  • Don't know if more tests are needed at a deeper level, but it seems all of the code is exercised except perhaps some assertions that are difficult to enfore (e.g. alignment).
  • @dexonsmith You mentioned that phabricator reviews can be linked. I'm having trouble finding out how.

Diff Detail

Event Timeline

wolfgangp created this revision.May 19 2022, 11:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 11:32 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
wolfgangp requested review of this revision.May 19 2022, 11:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 11:32 AM

You mentioned that phabricator reviews can be linked. I'm having trouble finding out how.

NM, found it.

wolfgangp updated this revision to Diff 431174.May 21 2022, 1:20 PM

Use the default constructor for LargeStorageVector followed by a resize instead of the constructor using a numOps parameter. Doing the latter requires the copy constructor for MDOperand.

dexonsmith added inline comments.May 24 2022, 12:33 PM
llvm/include/llvm/IR/Metadata.h
1223

It’d be good to have a header comment explaining when it’s legal to resize.

llvm/lib/IR/Metadata.cpp
579

Nit: you can reduce nesting by returning early here.

589

Nit: you can reduce nesting by returning early inside the if and skipping the else.

llvm/unittests/IR/MetadataTest.cpp
3663

Is this enough to exceed small size? If not, it’s be good to add one that does.

wolfgangp marked 4 inline comments as done.

Addressed review comments.

Added 2 new unit tests. One for an MDNode initially allocated as "large", and a death test for the case of a temporary node that is later uniqued. We want to make sure it is not resizable after it's been uniqued, even though it was originally allocated as resizable.

wolfgangp added inline comments.May 25 2022, 10:57 PM
llvm/unittests/IR/MetadataTest.cpp
3663

The node is allocated with 0 operands, so it gets resized to a large after only 3 push_backs But we were missing a test for a node that is allocated as a large right away. I added a test for it.

dexonsmith accepted this revision.May 26 2022, 7:47 AM

LGTM, thanks! I left a couple of nits inline for you to consider.

llvm/include/llvm/IR/Metadata.h
956–971

Nit: I suggest naming these Is{Resizable,Large}.

993–998

Nit: I'm not sure these accessors/mutators carry their weight, given that fields are already public in Header and Header is an implementation detail of MDOperand (no other users). I'd just as soon skip them and access the fields directly.

(If you have a short-term plan for a follow up that makes one of them non-trivial, maybe it'd be worth leaving that one behind, but otherwise...)

999

(Might skip this one too...)

This revision is now accepted and ready to land.May 26 2022, 7:47 AM
This revision was landed with ongoing or failed builds.Jun 7 2022, 2:36 PM
This revision was automatically updated to reflect the committed changes.

I am not surprised by the increased memory usage. The change introduces a resize capability to MDNodes, which when used more often in the future, should improve memory usage overall, especially with LTO. I am concerned about the compile time increase, which is a bit more than I expected. I reverted the patch for other reasons, but I'll check into runtime improvements before I reland.

This comment was removed by xbolva00.