[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 18:52:53 PDT 2016


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