From 50c7f32d75bdb7724547d39300048c54f23afdc9 Mon Sep 17 00:00:00 2001 From: "Duncan P. N. Exon Smith" Date: Fri, 8 Apr 2016 00:56:21 +0000 Subject: [PATCH] Revert "ValueMapper: Treat LocalAsMetadata more like function-local Values" This reverts commit r265759, since even this limited version breaks some bots: http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/3311 http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/17696 This also reverts r265761 "ValueMapper: Unduplicate RF_NoModuleLevelChanges check, NFC", since I had trouble separating it from r265759. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@265765 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/llvm/Transforms/Utils/ValueMapper.h | 42 ++------ lib/IR/Verifier.cpp | 4 - lib/Transforms/Utils/ValueMapper.cpp | 83 +++++----------- .../local-as-metadata-undominated-use.ll | 49 ---------- .../Transforms/Utils/ValueMapperTest.cpp | 95 ------------------- 5 files changed, 30 insertions(+), 243 deletions(-) delete mode 100644 test/Transforms/Inline/local-as-metadata-undominated-use.ll diff --git a/include/llvm/Transforms/Utils/ValueMapper.h b/include/llvm/Transforms/Utils/ValueMapper.h index e4dedfe699c..e8023bd3df9 100644 --- a/include/llvm/Transforms/Utils/ValueMapper.h +++ b/include/llvm/Transforms/Utils/ValueMapper.h @@ -68,21 +68,13 @@ enum RemapFlags { RF_NoModuleLevelChanges = 1, /// If this flag is set, the remapper ignores missing function-local entries - /// (Argument, Instruction, BasicBlock) that are not in the value map. If it - /// is unset, it aborts if an operand is asked to be remapped which doesn't - /// exist in the mapping. + /// (Argument, Instruction, BasicBlock) that are not in the + /// value map. If it is unset, it aborts if an operand is asked to be + /// remapped which doesn't exist in the mapping. /// - /// There are no such assertions in MapValue(), whose results are almost - /// unchanged by this flag. This flag mainly changes the assertion behaviour - /// in RemapInstruction(). - /// - /// Since an Instruction's metadata operands (even that point to SSA values) - /// aren't guaranteed to be dominated by their definitions, MapMetadata will - /// return "!{}" instead of "null" for \a LocalAsMetadata instances whose SSA - /// values are unmapped when this flag is set. Otherwise, \a MapValue() - /// completely ignores this flag. - /// - /// \a MapMetadata() always ignores this flag. + /// There are no such assertions in MapValue(), whose result should be + /// essentially unchanged by this flag. This only changes the assertion + /// behaviour in RemapInstruction(). RF_IgnoreMissingLocals = 2, /// Instruct the remapper to move distinct metadata instead of duplicating it @@ -109,32 +101,12 @@ static inline RemapFlags operator|(RemapFlags LHS, RemapFlags RHS) { /// 3. Else if \c V is a function-local value, return nullptr. /// 4. Else if \c V is a \a GlobalValue, return \c nullptr or \c V depending /// on \a RF_NullMapMissingGlobalValues. -/// 5. Else if \c V is a \a MetadataAsValue wrapping a LocalAsMetadata, -/// recurse on the local SSA value, and return nullptr or "metadata !{}" on -/// missing depending on RF_IgnoreMissingValues. -/// 6. Else if \c V is a \a MetadataAsValue, rewrap the return of \a -/// MapMetadata(). -/// 7. Else, compute the equivalent constant, and return it. +/// 5. Else, Compute the equivalent constant, and return it. Value *MapValue(const Value *V, ValueToValueMapTy &VM, RemapFlags Flags = RF_None, ValueMapTypeRemapper *TypeMapper = nullptr, ValueMaterializer *Materializer = nullptr); -/// Lookup or compute a mapping for a piece of metadata. -/// -/// Compute and memoize a mapping for \c MD. -/// -/// 1. If \c MD is mapped, return it. -/// 2. Else if \a RF_NoModuleLevelChanges or \c MD is an \a MDString, return -/// \c MD. -/// 3. Else if \c MD is a \a ConstantAsMetadata, call \a MapValue() and -/// re-wrap its return (returning nullptr on nullptr). -/// 4. Else, \c MD is an \a MDNode. These are remapped, along with their -/// transitive operands. Distinct nodes are duplicated or moved depending -/// on \a RF_MoveDistinctNodes. Uniqued nodes are remapped like constants. -/// -/// \note \a LocalAsMetadata is completely unsupported by \a MapMetadata. -/// Instead, use \a MapValue() with its wrapping \a MetadataAsValue instance. Metadata *MapMetadata(const Metadata *MD, ValueToValueMapTy &VM, RemapFlags Flags = RF_None, ValueMapTypeRemapper *TypeMapper = nullptr, diff --git a/lib/IR/Verifier.cpp b/lib/IR/Verifier.cpp index c0fb8fcbe07..b6b5f1a12c9 100644 --- a/lib/IR/Verifier.cpp +++ b/lib/IR/Verifier.cpp @@ -3502,10 +3502,6 @@ void Verifier::verifyDominatesUse(Instruction &I, unsigned i) { // Quick check whether the def has already been encountered in the same block. // PHI nodes are not checked to prevent accepting preceeding PHIs, because PHI // uses are defined to happen on the incoming edge, not at the instruction. - // - // FIXME: If this operand is a MetadataAsValue (wrapping a LocalAsMetadata) - // wrapping an SSA value, assert that we've already encountered it. See - // related FIXME in Mapper::mapLocalAsMetadata in ValueMapper.cpp. if (!isa(I) && InstsInThisBlock.count(Op)) return; diff --git a/lib/Transforms/Utils/ValueMapper.cpp b/lib/Transforms/Utils/ValueMapper.cpp index 27c7f1b3490..59870f81c3b 100644 --- a/lib/Transforms/Utils/ValueMapper.cpp +++ b/lib/Transforms/Utils/ValueMapper.cpp @@ -86,20 +86,6 @@ 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); @@ -308,32 +294,18 @@ Value *Mapper::mapValue(const Value *V) { if (const auto *MDV = dyn_cast(V)) { const Metadata *MD = MDV->getMetadata(); - - if (auto *LAM = dyn_cast(MD)) { - // Look through to grab the local value. - if (Value *LV = mapValue(LAM->getValue())) { - if (V == LAM->getValue()) - return const_cast(V); - return MetadataAsValue::get(V->getContext(), LocalAsMetadata::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 (Flags & RF_NoModuleLevelChanges) + if (!isa(MD) && (Flags & RF_NoModuleLevelChanges)) return VM[V] = const_cast(V); - // Map the metadata and turn it into a value. + // FIXME: be consistent with function-local values for LocalAsMetadata by + // returning nullptr when LocalAsMetadata is missing. Adding a mapping is + // expensive. auto *MappedMD = mapMetadata(MD); - if (MD == MappedMD) + if (MD == MappedMD || (!MappedMD && (Flags & RF_IgnoreMissingLocals))) return VM[V] = const_cast(V); + return VM[V] = MetadataAsValue::get(V->getContext(), MappedMD); } @@ -651,18 +623,21 @@ Optional Mapper::mapSimpleMetadata(const Metadata *MD) { if (isa(MD)) 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); + if (isa(MD)) + if ((Flags & RF_NoModuleLevelChanges)) + return mapToSelf(MD); - if (auto *CMD = dyn_cast(MD)) { + // FIXME: Assert that this is not LocalAsMetadata. It should be handled + // elsewhere. + if (const auto *VMD = dyn_cast(MD)) { // Disallow recursion into metadata mapping through mapValue. VM.disableMapMetadata(); - Value *MappedV = mapValue(CMD->getValue()); + Value *MappedV = mapValue(VMD->getValue()); VM.enableMapMetadata(); - if (CMD->getValue() == MappedV) + // FIXME: Always use "ignore" behaviour. There should only be globals here. + if (VMD->getValue() == MappedV || + (!MappedV && (Flags & RF_IgnoreMissingLocals))) return mapToSelf(MD); return mapToMetadata(MD, MappedV ? ValueAsMetadata::get(MappedV) : nullptr); @@ -670,6 +645,11 @@ Optional Mapper::mapSimpleMetadata(const Metadata *MD) { assert(isa(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; } @@ -679,26 +659,9 @@ 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(&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) { - assert(MD && "Expected valid metadata"); - assert(!isa(MD) && "Unexpected local metadata"); - + // FIXME: First check for and deal with LocalAsMetadata, so that + // mapSimpleMetadata() doesn't need to deal with it. if (Optional NewMD = mapSimpleMetadata(MD)) return *NewMD; diff --git a/test/Transforms/Inline/local-as-metadata-undominated-use.ll b/test/Transforms/Inline/local-as-metadata-undominated-use.ll deleted file mode 100644 index 5182e2148dc..00000000000 --- a/test/Transforms/Inline/local-as-metadata-undominated-use.ll +++ /dev/null @@ -1,49 +0,0 @@ -; RUN: opt -inline -S < %s | FileCheck %s - -; Make sure the inliner doesn't crash when a metadata-bridged SSA operand is an -; undominated use. -; -; If we ever add a verifier check to prevent the scenario in this file, it's -; fine to delete this testcase. However, we would need a bitcode upgrade since -; such historical IR exists in practice. - -define i32 @foo(i32 %i) !dbg !4 { -entry: - tail call void @llvm.dbg.value(metadata i32 %add, i64 0, metadata !8, metadata !10), !dbg !11 - %add = add nsw i32 1, %i, !dbg !12 - ret i32 %add, !dbg !13 -} - -; CHECK-LABEL: define i32 @caller( -define i32 @caller(i32 %i) { -; CHECK-NEXT: entry: -entry: -; Although the inliner shouldn't crash, it can't be expected to get the -; "correct" SSA value since its assumptions have been violated. -; CHECK-NEXT: tail call void @llvm.dbg.value(metadata ![[EMPTY:[0-9]+]], -; CHECK-NEXT: %{{.*}} = add nsw - %call = tail call i32 @foo(i32 %i) - ret i32 %call -} - -declare void @llvm.dbg.value(metadata, i64, metadata, metadata) - -!llvm.dbg.cu = !{!0} -!llvm.module.flags = !{!9} - -!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 3.9.0 (trunk 265634) (llvm/trunk 265637)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, subprograms: !3) -!1 = !DIFile(filename: "t.c", directory: "/path/to/tests") - -; CHECK: ![[EMPTY]] = !{} -!2 = !{} -!3 = !{!4} -!4 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 2, type: !5, isLocal: false, isDefinition: true, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: true) -!5 = !DISubroutineType(types: !6) -!6 = !{!7, !7} -!7 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed) -!8 = !DILocalVariable(name: "add", arg: 1, scope: !4, file: !1, line: 2, type: !7) -!9 = !{i32 2, !"Debug Info Version", i32 3} -!10 = !DIExpression() -!11 = !DILocation(line: 2, column: 13, scope: !4) -!12 = !DILocation(line: 2, column: 27, scope: !4) -!13 = !DILocation(line: 2, column: 18, scope: !4) diff --git a/unittests/Transforms/Utils/ValueMapperTest.cpp b/unittests/Transforms/Utils/ValueMapperTest.cpp index 151f76da9cb..222f2a2e863 100644 --- a/unittests/Transforms/Utils/ValueMapperTest.cpp +++ b/unittests/Transforms/Utils/ValueMapperTest.cpp @@ -139,99 +139,4 @@ TEST(ValueMapperTest, MapMetadataNullMapGlobalWithIgnoreMissingLocals) { EXPECT_EQ(nullptr, MapValue(F.get(), VM, Flags)); } -TEST(ValueMapperTest, MapMetadataConstantAsMetadata) { - LLVMContext C; - FunctionType *FTy = - FunctionType::get(Type::getVoidTy(C), Type::getInt8Ty(C), false); - std::unique_ptr F( - Function::Create(FTy, GlobalValue::ExternalLinkage, "F")); - - auto *CAM = ConstantAsMetadata::get(F.get()); - { - ValueToValueMapTy VM; - EXPECT_EQ(CAM, MapMetadata(CAM, VM)); - EXPECT_TRUE(VM.MD().count(CAM)); - VM.MD().erase(CAM); - EXPECT_EQ(CAM, MapMetadata(CAM, VM, RF_IgnoreMissingLocals)); - EXPECT_TRUE(VM.MD().count(CAM)); - - auto *N = MDTuple::get(C, None); - VM.MD()[CAM].reset(N); - EXPECT_EQ(N, MapMetadata(CAM, VM)); - EXPECT_EQ(N, MapMetadata(CAM, VM, RF_IgnoreMissingLocals)); - } - - std::unique_ptr F2( - Function::Create(FTy, GlobalValue::ExternalLinkage, "F2")); - ValueToValueMapTy VM; - VM[F.get()] = F2.get(); - auto *F2MD = MapMetadata(CAM, VM); - EXPECT_TRUE(VM.MD().count(CAM)); - EXPECT_TRUE(F2MD); - EXPECT_EQ(F2.get(), cast(F2MD)->getValue()); -} - -#ifdef GTEST_HAS_DEATH_TEST -#ifndef NDEBUG -TEST(ValueMapperTest, MapMetadataLocalAsMetadata) { - LLVMContext C; - FunctionType *FTy = - FunctionType::get(Type::getVoidTy(C), Type::getInt8Ty(C), false); - std::unique_ptr F( - Function::Create(FTy, GlobalValue::ExternalLinkage, "F")); - Argument &A = *F->arg_begin(); - - // MapMetadata doesn't support LocalAsMetadata. The only valid container for - // LocalAsMetadata is a MetadataAsValue instance, so use it directly. - auto *LAM = LocalAsMetadata::get(&A); - ValueToValueMapTy VM; - EXPECT_DEATH(MapMetadata(LAM, VM), "Unexpected local metadata"); - EXPECT_DEATH(MapMetadata(LAM, VM, RF_IgnoreMissingLocals), - "Unexpected local metadata"); -} -#endif -#endif - -TEST(ValueMapperTest, MapValueLocalAsMetadata) { - LLVMContext C; - FunctionType *FTy = - FunctionType::get(Type::getVoidTy(C), Type::getInt8Ty(C), false); - std::unique_ptr F( - Function::Create(FTy, GlobalValue::ExternalLinkage, "F")); - Argument &A = *F->arg_begin(); - - auto *LAM = LocalAsMetadata::get(&A); - auto *MAV = MetadataAsValue::get(C, LAM); - - // The principled answer to a LocalAsMetadata of an unmapped SSA value would - // be to return nullptr (regardless of RF_IgnoreMissingLocals). - // - // However, algorithms that use RemapInstruction assume that each instruction - // only references SSA values from previous instructions. Arguments of - // such as "metadata i32 %x" don't currently successfully maintain that - // property. To keep RemapInstruction from crashing we need a non-null - // return here, but we also shouldn't reference the unmapped local. Use - // "metadata !{}". - auto *N0 = MDTuple::get(C, None); - auto *N0AV = MetadataAsValue::get(C, N0); - ValueToValueMapTy VM; - EXPECT_EQ(N0AV, MapValue(MAV, VM)); - EXPECT_EQ(nullptr, MapValue(MAV, VM, RF_IgnoreMissingLocals)); - EXPECT_FALSE(VM.count(MAV)); - EXPECT_FALSE(VM.count(&A)); - EXPECT_EQ(None, VM.getMappedMD(LAM)); - - VM[MAV] = MAV; - EXPECT_EQ(MAV, MapValue(MAV, VM)); - EXPECT_EQ(MAV, MapValue(MAV, VM, RF_IgnoreMissingLocals)); - EXPECT_TRUE(VM.count(MAV)); - EXPECT_FALSE(VM.count(&A)); - - VM[MAV] = &A; - EXPECT_EQ(&A, MapValue(MAV, VM)); - EXPECT_EQ(&A, MapValue(MAV, VM, RF_IgnoreMissingLocals)); - EXPECT_TRUE(VM.count(MAV)); - EXPECT_FALSE(VM.count(&A)); -} - } // end namespace