This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Metadata linking for imported functions
ClosedPublic

Authored by tejohnson on Nov 19 2015, 12:13 PM.

Details

Summary

Second patch split out from http://reviews.llvm.org/D14752.

Maps metadata as a post-pass from each module when importing complete,
suturing up final metadata to the temporary metadata left on the
imported instructions.

This entails saving the mapping from bitcode value id to temporary
metadata in the importing pass, and from bitcode value id to final
metadata during the metadata linking postpass.

Depends on D14825.

Diff Detail

Repository
rL LLVM

Event Timeline

tejohnson updated this revision to Diff 40690.Nov 19 2015, 12:13 PM
tejohnson retitled this revision from to [ThinLTO] Metadata linking for imported functions.
tejohnson updated this object.
tejohnson added reviewers: dexonsmith, mehdi_amini.
tejohnson added subscribers: llvm-commits, davidxl.
tejohnson updated this revision to Diff 40701.Nov 19 2015, 1:46 PM
  • Remove a change that shouldn't be in this patch.

Ping.
Thanks, Teresa

mehdi_amini edited edge metadata.Dec 1 2015, 12:18 PM

It looks good to me, with a few minor comments.
I'd rather have another pair of eyes on it since I'm not sure I understand everything though.

Thanks!

include/llvm/IR/Metadata.h
833 ↗(On Diff #40701)

I think we are usually using \p instead of \a when referring to parameters

lib/Bitcode/Reader/BitcodeReader.cpp
3116 ↗(On Diff #40701)

I'm reluctant to i as a variable name, maybe unsigned CurrentValueID instead?

3128 ↗(On Diff #40701)

I'm not sure about this early exit?

lib/Linker/LinkModules.cpp
506 ↗(On Diff #40701)

Suggestion: ValueMapperFlags |= RF_HaveUnmaterializedMetadata;

994 ↗(On Diff #40701)

early return:

if (!ValIDToTempMDMap) return nullptr;
1013 ↗(On Diff #40701)

same early return: if (!ValIDToTempMDMap) return;

2251 ↗(On Diff #40701)

Comment?

tools/llvm-link/llvm-link.cpp
169 ↗(On Diff #40701)
StringMap<std::unique_ptr<DenseMap<unsigned, MDNode *>>> ModuleToTempMDValsMap;
229 ↗(On Diff #40701)

May I suggest:

auto &TempMDVals = ModuleToTempMDValsMap[FileName];
if (!TempMDVals)
  TempMDVals = new DenseMap<unsigned, MDNode *>();

(one query to the map)

Thanks for the comments. Will upload new patch after testing.

include/llvm/IR/Metadata.h
833 ↗(On Diff #40701)

Fixed.

lib/Bitcode/Reader/BitcodeReader.cpp
3116 ↗(On Diff #40701)

Renamed to ValID.

3128 ↗(On Diff #40701)

Good catch, should be a continue. This isn't triggered with current HEAD since it only would affect the case where we import >1 function at a time, which is obviously in progress.

I added another change to set the SavedFwdRefs flag to true whenever we add a forward ref which should allow us to catch any case where we miss saving off any forward references (e.g. if this bug wasn't fixed).

lib/Linker/LinkModules.cpp
506 ↗(On Diff #40701)

I did that initially because the |= form gives me:

error: assigning to 'llvm::RemapFlags' from incompatible type 'int'

No amount of casting the RF_ value to the enum type gets me past this error. So I have left it as is for now. Suggestions?

994 ↗(On Diff #40701)

Done

1013 ↗(On Diff #40701)

Done

2251 ↗(On Diff #40701)

Done

tools/llvm-link/llvm-link.cpp
169 ↗(On Diff #40701)

Good idea, done

229 ↗(On Diff #40701)

Done

tejohnson updated this revision to Diff 42191.Dec 8 2015, 10:36 AM
tejohnson edited edge metadata.
  • Address comments from Mehdi and Duncan.
mehdi_amini accepted this revision.Dec 11 2015, 2:09 PM
mehdi_amini edited edge metadata.

LGTM

lib/Linker/LinkModules.cpp
625 ↗(On Diff #42191)

commit separately if possible

This revision is now accepted and ready to land.Dec 11 2015, 2:09 PM
This revision was automatically updated to reflect the committed changes.