[llvm] r265765 - Revert "ValueMapper: Treat LocalAsMetadata more like function-local Values"
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 7 19:28:47 PDT 2016
Hmm... I just spawned a build of r265759 on Darwin and it causes a failure
there too:
http://lab.llvm.org:8080/green/job/clang-stage1-configure-RA_build/19020
However, I'd still appreciate one of the crash reproducers from your bot.
I'd like to be sure it's the same issue!
> On 2016-Apr-07, at 21:52, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>
> Bill,
>
> Would you please send me the crash reproductions form r265759? Maybe
> attach them to PR27273?
>
> http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/3311
>
> Thanks!
>
>> On 2016-Apr-07, at 20:56, Duncan P. N. Exon Smith via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>
>> Author: dexonsmith
>> Date: Thu Apr 7 19:56:21 2016
>> New Revision: 265765
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=265765&view=rev
>> Log:
>> 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.
>>
>> Removed:
>> llvm/trunk/test/Transforms/Inline/local-as-metadata-undominated-use.ll
>> Modified:
>> llvm/trunk/include/llvm/Transforms/Utils/ValueMapper.h
>> llvm/trunk/lib/IR/Verifier.cpp
>> llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp
>> llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp
>>
>> Modified: llvm/trunk/include/llvm/Transforms/Utils/ValueMapper.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Transforms/Utils/ValueMapper.h?rev=265765&r1=265764&r2=265765&view=diff
>> ==============================================================================
>> --- llvm/trunk/include/llvm/Transforms/Utils/ValueMapper.h (original)
>> +++ llvm/trunk/include/llvm/Transforms/Utils/ValueMapper.h Thu Apr 7 19:56:21 2016
>> @@ -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|(Remap
>> /// 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,
>>
>> Modified: llvm/trunk/lib/IR/Verifier.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Verifier.cpp?rev=265765&r1=265764&r2=265765&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/IR/Verifier.cpp (original)
>> +++ llvm/trunk/lib/IR/Verifier.cpp Thu Apr 7 19:56:21 2016
>> @@ -3502,10 +3502,6 @@ void Verifier::verifyDominatesUse(Instru
>> // 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<PHINode>(I) && InstsInThisBlock.count(Op))
>> return;
>>
>>
>> Modified: llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp?rev=265765&r1=265764&r2=265765&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp Thu Apr 7 19:56:21 2016
>> @@ -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<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(), 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<LocalAsMetadata>(MD) && (Flags & RF_NoModuleLevelChanges))
>> return VM[V] = const_cast<Value *>(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<Value *>(V);
>> +
>> return VM[V] = MetadataAsValue::get(V->getContext(), MappedMD);
>> }
>>
>> @@ -651,18 +623,21 @@ Optional<Metadata *> Mapper::mapSimpleMe
>> if (isa<MDString>(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<ConstantAsMetadata>(MD))
>> + if ((Flags & RF_NoModuleLevelChanges))
>> + return mapToSelf(MD);
>>
>> - if (auto *CMD = dyn_cast<ConstantAsMetadata>(MD)) {
>> + // FIXME: Assert that this is not LocalAsMetadata. It should be handled
>> + // elsewhere.
>> + if (const auto *VMD = dyn_cast<ValueAsMetadata>(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<Metadata *> Mapper::mapSimpleMe
>>
>> 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;
>> }
>>
>> @@ -679,26 +659,9 @@ Metadata *llvm::MapMetadata(const Metada
>> 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) {
>> - assert(MD && "Expected valid metadata");
>> - assert(!isa<LocalAsMetadata>(MD) && "Unexpected local metadata");
>> -
>> + // FIXME: First check for and deal with LocalAsMetadata, so that
>> + // mapSimpleMetadata() doesn't need to deal with it.
>> if (Optional<Metadata *> NewMD = mapSimpleMetadata(MD))
>> return *NewMD;
>>
>>
>> Removed: llvm/trunk/test/Transforms/Inline/local-as-metadata-undominated-use.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/local-as-metadata-undominated-use.ll?rev=265764&view=auto
>> ==============================================================================
>> --- llvm/trunk/test/Transforms/Inline/local-as-metadata-undominated-use.ll (original)
>> +++ llvm/trunk/test/Transforms/Inline/local-as-metadata-undominated-use.ll (removed)
>> @@ -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)
>>
>> Modified: llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp?rev=265765&r1=265764&r2=265765&view=diff
>> ==============================================================================
>> --- llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp (original)
>> +++ llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp Thu Apr 7 19:56:21 2016
>> @@ -139,99 +139,4 @@ TEST(ValueMapperTest, MapMetadataNullMap
>> 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<Function> 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<Function> 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<ConstantAsMetadata>(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<Function> 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<Function> 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
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list