Reapply "ValueMapper: Treat LocalAsMetadata more like function-local Values"
This reverts commit r265765, reapplying r265759 after changing a call from
LocalAsMetadata::get to ValueAsMetadata::get (and adding a unit test). When a
local value is mapped to a constant (like "i32 %a" => "i32 7"), the new debug
intrinsic operand may no longer be pointing at a local.
http://lab.llvm.org:8080/green/job/clang-stage1-configure-RA_build/19020/
The previous coommit message follows:
--
This is a partial re-commit -- maybe more of a re-implementation -- of
r265631 (reverted in r265637).
This makes RF_IgnoreMissingLocals behave (almost) consistently between
the Value and the Metadata hierarchy. In particular:
- MapValue returns nullptr or "metadata !{}" for missing locals in
MetadataAsValue/LocalAsMetadata bridging paris, depending on
the RF_IgnoreMissingLocals flag.
- MapValue doesn't memoize LocalAsMetadata-related results.
- MapMetadata no longer deals with LocalAsMetadata or
RF_IgnoreMissingLocals at all. (This wasn't in r265631 at all, but
I realized during testing it would make the patch simpler with no
loss of generality.)
r265631 went too far, making both functions universally ignore
RF_IgnoreMissingLocals. This broke building (e.g.) compiler-rt.
Reassociate (and possibly other passes) don't currently maintain
dominates-use invariants for metadata operands, resulting in IR like
this:
define void @foo(i32 %arg) {
call void @llvm.some.intrinsic(metadata i32 %x)
%x = add i32 1, i32 %arg
}
If the inliner chooses to inline @foo into another function, then
RemapInstruction will call `MapValue(metadata i32 %x)` and assert that
the return is not nullptr.
I've filed PR27273 to add a Verifier check and fix the underlying
problem in the optimization passes.
As a workaround, return `!{}` instead of nullptr for unmapped
LocalAsMetadata when RF_IgnoreMissingLocals is unset. Otherwise, match
the behaviour of r265631.
Original commit message:
ValueMapper: Make LocalAsMetadata match function-local Values
Start treating LocalAsMetadata similarly to function-local members of
the Value hierarchy in MapValue and MapMetadata.
- Don't memoize them.
- Return nullptr if they are missing.
This also cleans up ConstantAsMetadata to stop listening to the
RF_IgnoreMissingLocals flag.
llvm-svn: 265768
diff --git a/llvm/lib/Transforms/Utils/ValueMapper.cpp b/llvm/lib/Transforms/Utils/ValueMapper.cpp
index 59870f8..a598111 100644
--- a/llvm/lib/Transforms/Utils/ValueMapper.cpp
+++ b/llvm/lib/Transforms/Utils/ValueMapper.cpp
@@ -86,6 +86,20 @@
/// (not an MDNode, or MDNode::isResolved() returns true).
Metadata *mapMetadata(const Metadata *MD);
+ // Map LocalAsMetadata, which never gets memoized.
+ //
+ // If the referenced local is not mapped, the principled return is nullptr.
+ // However, optimization passes sometimes move metadata operands *before* the
+ // SSA values they reference. To prevent crashes in \a RemapInstruction(),
+ // return "!{}" when RF_IgnoreMissingLocals is not set.
+ //
+ // \note Adding a mapping for LocalAsMetadata is unsupported. Add a mapping
+ // to the value map for the SSA value in question instead.
+ //
+ // FIXME: Once we have a verifier check for forward references to SSA values
+ // through metadata operands, always return nullptr on unmapped locals.
+ Metadata *mapLocalAsMetadata(const LocalAsMetadata &LAM);
+
private:
Value *mapBlockAddress(const BlockAddress &BA);
@@ -294,18 +308,32 @@
if (const auto *MDV = dyn_cast<MetadataAsValue>(V)) {
const Metadata *MD = MDV->getMetadata();
+
+ if (auto *LAM = dyn_cast<LocalAsMetadata>(MD)) {
+ // Look through to grab the local value.
+ if (Value *LV = mapValue(LAM->getValue())) {
+ if (V == LAM->getValue())
+ return const_cast<Value *>(V);
+ return MetadataAsValue::get(V->getContext(), ValueAsMetadata::get(LV));
+ }
+
+ // FIXME: always return nullptr once Verifier::verifyDominatesUse()
+ // ensures metadata operands only reference defined SSA values.
+ return (Flags & RF_IgnoreMissingLocals)
+ ? nullptr
+ : MetadataAsValue::get(V->getContext(),
+ MDTuple::get(V->getContext(), None));
+ }
+
// If this is a module-level metadata and we know that nothing at the module
// level is changing, then use an identity mapping.
- if (!isa<LocalAsMetadata>(MD) && (Flags & RF_NoModuleLevelChanges))
+ if (Flags & RF_NoModuleLevelChanges)
return VM[V] = const_cast<Value *>(V);
- // FIXME: be consistent with function-local values for LocalAsMetadata by
- // returning nullptr when LocalAsMetadata is missing. Adding a mapping is
- // expensive.
+ // Map the metadata and turn it into a value.
auto *MappedMD = mapMetadata(MD);
- if (MD == MappedMD || (!MappedMD && (Flags & RF_IgnoreMissingLocals)))
+ if (MD == MappedMD)
return VM[V] = const_cast<Value *>(V);
-
return VM[V] = MetadataAsValue::get(V->getContext(), MappedMD);
}
@@ -623,21 +651,18 @@
if (isa<MDString>(MD))
return mapToSelf(MD);
- if (isa<ConstantAsMetadata>(MD))
- if ((Flags & RF_NoModuleLevelChanges))
- return mapToSelf(MD);
+ // This is a module-level metadata. If nothing at the module level is
+ // changing, use an identity mapping.
+ if ((Flags & RF_NoModuleLevelChanges))
+ return mapToSelf(MD);
- // FIXME: Assert that this is not LocalAsMetadata. It should be handled
- // elsewhere.
- if (const auto *VMD = dyn_cast<ValueAsMetadata>(MD)) {
+ if (auto *CMD = dyn_cast<ConstantAsMetadata>(MD)) {
// Disallow recursion into metadata mapping through mapValue.
VM.disableMapMetadata();
- Value *MappedV = mapValue(VMD->getValue());
+ Value *MappedV = mapValue(CMD->getValue());
VM.enableMapMetadata();
- // FIXME: Always use "ignore" behaviour. There should only be globals here.
- if (VMD->getValue() == MappedV ||
- (!MappedV && (Flags & RF_IgnoreMissingLocals)))
+ if (CMD->getValue() == MappedV)
return mapToSelf(MD);
return mapToMetadata(MD, MappedV ? ValueAsMetadata::get(MappedV) : nullptr);
@@ -645,11 +670,6 @@
assert(isa<MDNode>(MD) && "Expected a metadata node");
- // If this is a module-level metadata and we know that nothing at the
- // module level is changing, then use an identity mapping.
- if (Flags & RF_NoModuleLevelChanges)
- return mapToSelf(MD);
-
return None;
}
@@ -659,9 +679,26 @@
return Mapper(VM, Flags, TypeMapper, Materializer).mapMetadata(MD);
}
+Metadata *Mapper::mapLocalAsMetadata(const LocalAsMetadata &LAM) {
+ // Lookup the mapping for the value itself, and return the appropriate
+ // metadata.
+ if (Value *V = mapValue(LAM.getValue())) {
+ if (V == LAM.getValue())
+ return const_cast<LocalAsMetadata *>(&LAM);
+ return ValueAsMetadata::get(V);
+ }
+
+ // FIXME: always return nullptr once Verifier::verifyDominatesUse() ensures
+ // metadata operands only reference defined SSA values.
+ return (Flags & RF_IgnoreMissingLocals)
+ ? nullptr
+ : MDTuple::get(LAM.getContext(), None);
+}
+
Metadata *Mapper::mapMetadata(const Metadata *MD) {
- // FIXME: First check for and deal with LocalAsMetadata, so that
- // mapSimpleMetadata() doesn't need to deal with it.
+ assert(MD && "Expected valid metadata");
+ assert(!isa<LocalAsMetadata>(MD) && "Unexpected local metadata");
+
if (Optional<Metadata *> NewMD = mapSimpleMetadata(MD))
return *NewMD;