[PATCH] D96531: TransformUtils: Fix metadata handling in CloneModule (and improve CloneFunctionInto)

Thu Feb 11 11:18:19 PST 2021

dexonsmith created this revision.
dexonsmith added reviewers: aprantl, hans, ruiling, loladiro.
Herald added subscribers: lxfind, okura, kuter, kerbowa, hiraditya, nhaehnle, jvesely, arsenm.
dexonsmith requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added a reviewer: sstefan1.
Herald added a reviewer: baziotis.
Herald added a subscriber: bbn.
Herald added a project: LLVM.

This commit fixes how metadata is handled in CloneModule to be sound,
and improves how it's handled in CloneFunctionInto (although the latter
is still not quite right).

Ruiling Song pointed out in PR48841 that CloneModule was changed to
unsoundly use the RF_ReuseAndMutateDistinctMDs flag (renamed in
fa35c1f80f0ea080a7cbc581416929b0a654f25c <https://reviews.llvm.org/rGfa35c1f80f0ea080a7cbc581416929b0a654f25c> for clarity). This flag papered
over a crash caused by other various changes made to CloneFunctionInto
over the past few years that made it unsound to use cloning between
different modules. I don't know if this patch fixes PR48841, but it does
fix a few soundness issues that have crept in.

RF_ReuseAndMutateDistinctMDs is designed for the IRMover to use,
avoiding unnecessary clones of all referenced metadata when linking
between modules (with IRMover, the source module is discarded after
linking). It never makes sense to use when you're not discarding the
source. This patch drops its incorrect use in CloneModule.

Sadly, the right thing to do with metadata when cloning a function is
complicated, and this patch doesn't totally fix it.

The first problem is that there are two different types of referenceable
metadata and it's not obvious what to with one of them when remapping.

- `!0 = !{!1}` is metadata's version of a constant. Programatically it's called "uniqued" (probably a better term would be "constant") because, like `ConstantArray`, it's stored in uniquing tables. Once it's constructed, it's illegal to change its arguments.
- `!0 = distinct !{!1}` is a bit closer to a global variable. It's legal to change the operands after construction.

What should be done with distinct metadata when cloning functions within
the same module?

- Should new, cloned nodes be created?
- Should all references point to the same, old nodes?

The answer depends on whether that metadata is effectively owned by a
function.

And that's the second problem. Referenceable metadata's ownership model
is not clear or explicit. Technically, it's all stored on an
LLVMContext. However, any metadata that is `distinct`, that transitively
references a `distinct` node, or that transitively references a
GlobalValue is specific to a Module and is effectively owned by it. More
specifically, some metadata is effectively owned by a specific Function
within a module.

Effectively function-local metadata was introduced somewhere around
c10d0e5ccd12f049bddb24dcf8bbb7fbbc6c68f2, which made it illegal for two
functions to share a DISubprogram attachment.

When cloning a function within a module, you need to clone the
function-local debug info and suppress cloning of global debug info (the
status quo suppresses cloning some global debug info but not all). When
cloning a function to a new/different module, you need to clone all of
the debug info.

Here's what I think we should do (eventually? soon? not this patch
though):

- Distinguish explicitly (somehow) between pure constant metadata owned by the LLVMContext, global metadata owned by the Module, and local metadata owned by a GlobalValue (such as a function).
- Update CloneFunctionInto to trigger cloning of all "local" metadata (only), perhaps by adding a bit to RemapFlag. Alternatively, split out a separate function CloneFunctionMetadataInto to prime the metadata map that callers are updated to call ahead of time as appropriate.

Here's the somewhat more isolated fix in this patch:

- Converted the `ModuleLevelChanges` parameter to `CloneFunctionInto` to an enum called `CloneFunctionChangeType` that is one of LocalChangesOnly, GlobalChanges, DifferentModule, and ClonedModule.
- The code maintaining the "functions uniquely own subprograms" invariant is now only active in the first two cases, where a function is being cloned within a single module. That's necessary because this code inhibits cloning of (some) "global" metadata that's effectively owned by the module.
- The code maintaining the "all compile units must be explicitly referenced by !llvm.dbg.cu" invariant is now only active in the DifferentModule case, where a function is being cloned into a new module in isolation.
- CoroSplit.cpp's call to CloneFunctionInto in CoroCloner::create uses LocalChangeOnly, since fa635d730f74f3285b77cc1537f1692184b8bf5b only set `ModuleLevelChanges` to trigger cloning of local metadata.
- CloneModule drops its unsound use of RF_ReuseAndMutateDistinctMDs and special handling of !llvm.dbg.cu.
- Fixed some outdated header docs and left a couple of FIXMEs.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96531

Files:
  llvm/include/llvm/Transforms/Utils/Cloning.h
  llvm/lib/ExecutionEngine/Orc/IndirectionUtils.cpp
  llvm/lib/Target/AMDGPU/R600OpenCLImageTypeLoweringPass.cpp
  llvm/lib/Transforms/Coroutines/CoroSplit.cpp
  llvm/lib/Transforms/IPO/Attributor.cpp
  llvm/lib/Transforms/Utils/CloneFunction.cpp
  llvm/lib/Transforms/Utils/CloneModule.cpp
  llvm/unittests/Transforms/Utils/CloningTest.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D96531.323089.patch
Type: text/x-patch
Size: 20021 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210211/ba173afe/attachment.bin>