This is an archive of the discontinued LLVM Phabricator instance.

Transforms: Clone distinct nodes in metadata mapper unless RF_ReuseAndMutateDistinctMDs
ClosedPublic

Authored by dexonsmith on Feb 15 2021, 3:31 PM.

Details

Summary

This is a follow up to 22a52dfddcefad4f275eb8ad1cc0e200074c2d8a and a
revert of df763188c9a1ecb1e7e5c4d4ea53a99fbb755903.

With this change, we only skip cloning distinct nodes in
MDNodeMapper::mapDistinct if RF_ReuseAndMutateDistinctMDs, dropping the
no-longer-needed local helper cloneOrBuildODR(). Skipping cloning in
other cases is unsound and breaks CloneModule, which is why the textual
IR for PR48841 didn't pass previously. This commit adds the test as:
Transforms/ThinLTOBitcodeWriter/cfi-debug-info-cloned-type-references-global-value.ll

Cloning less often exposed a hole in subprogram cloning in
CloneFunctionInto thanks to df763188c9a1ecb1e7e5c4d4ea53a99fbb755903's
test ThinLTO/X86/Inputs/dicompositetype-unique-alias.ll. If a function
has a subprogram attachment whose scope is a DICompositeType that
shouldn't be cloned, but it has no internal debug info pointing at that
type, that composite type was being cloned. This commit plugs that hole,
calling DebugInfoFinder::processSubprogram from CloneFunctionInto.

As hinted at in 22a52dfddcefad4f275eb8ad1cc0e200074c2d8a's commit
message, I think we need to formalize ownership of metadata a bit more
so that ValueMapper/CloneFunctionInto (and similar functions) can deal
with cloning (or not) metadata in a more generic, less fragile way.

This fixes PR48841.

Follow-up to: https://reviews.llvm.org/D96531
Revert of: https://reviews.llvm.org/D41669

Diff Detail

Event Timeline

dexonsmith created this revision.Feb 15 2021, 3:31 PM
dexonsmith requested review of this revision.Feb 15 2021, 3:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2021, 3:31 PM
dexonsmith edited the summary of this revision. (Show Details)Feb 15 2021, 3:43 PM

Thanks. I am not very familiar with processSubprogram() or the implication of the other changes in CloneFunction.cpp, so will leave it to @aprantl or someone more familiar with the debug info details to review. I just have a small suggestion on the revert part.

llvm/lib/Transforms/Utils/ValueMapper.cpp
546

Why not just call MDNode::replaceWithDistinct(N.clone()) directly?

Drop the one-line helper, cloneDistinct().

dexonsmith marked an inline comment as done.Feb 15 2021, 5:11 PM
dexonsmith added inline comments.
llvm/lib/Transforms/Utils/ValueMapper.cpp
546

I had imagined this logic would get too busy, but actually this is fine. Thanks for the suggestion.

dexonsmith edited the summary of this revision. (Show Details)Feb 15 2021, 5:12 PM
aprantl accepted this revision.Feb 17 2021, 9:21 AM
This revision is now accepted and ready to land.Feb 17 2021, 9:21 AM