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

1 files changed, 60 insertions, 23 deletions

diff --git a/llvm/lib/Transforms/Utils/ValueMapper.cpp b/llvm/lib/Transforms/Utils/ValueMapper.cpp
index 59870f81c3b..a5981115584 100644
--- a/llvm/lib/Transforms/Utils/ValueMapper.cpp
+++ b/llvm/lib/Transforms/Utils/ValueMapper.cpp
@@ -86,6 +86,20 @@ public:
   /// (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 @@ Value *Mapper::mapValue(const Value *V) {
 
   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 @@ Optional<Metadata *> Mapper::mapSimpleMetadata(const Metadata *MD) {
   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 @@ Optional<Metadata *> Mapper::mapSimpleMetadata(const Metadata *MD) {
 
   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 @@ Metadata *llvm::MapMetadata(const Metadata *MD, ValueToValueMapTy &VM,
   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;