[PATCH] D43199: [IRMover] Implement name based structure type mapping

[Rafael Avila de Espindola via llvm-commits]

This now LGTM, but please get a LGTM from an extra developer. I would
suggest David Blaikie or Peter Collingbourne.

It is a pity that names are made even more significant, but I can't
think of a workable solution other than finishing the transition to a
single pointer type and that is way too much work for now.

The testcase that found the problem with my idea of editing the types in
the destination module was:

-----------------------------------------------
; RUN: echo > %t
; RUN: llvm-link --initial-module=%s %t -S -o - | FileCheck %s

%zed = type { i64 }
@x = global { i64 } zeroinitializer, align 8
define void @foo(%zed** %bar) {
  store %zed* bitcast ({ i64 }* @x to %zed*), %zed** %bar, align 8
  ret void
}
-----------------------------------------------

You might want to include that.

Cheers,
Rafael

Eugene Leviant via Phabricator <reviews at reviews.llvm.org> writes:

> This revision was automatically updated to reflect the committed changes.
> Closed by commit rL325686: [IRMover] Implement name based structure type mapping (authored by evgeny777, committed by ).
>
> Changed prior to commit:
>   https://reviews.llvm.org/D43199?vs=134444&id=135248#toc
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D43199
>
> Files:
>   llvm/trunk/include/llvm/Linker/IRMover.h
>   llvm/trunk/lib/Linker/IRMover.cpp
>   llvm/trunk/test/Linker/Inputs/struct-mapping.ll
>   llvm/trunk/test/Linker/struct-mapping.ll
>   llvm/trunk/tools/llvm-link/llvm-link.cpp
>
> Index: llvm/trunk/include/llvm/Linker/IRMover.h
> ===================================================================
> --- llvm/trunk/include/llvm/Linker/IRMover.h
> +++ llvm/trunk/include/llvm/Linker/IRMover.h
> @@ -49,17 +49,23 @@
>      // The set of opaque types is the composite module.
>      DenseSet<StructType *> OpaqueStructTypes;
>  
> -    // The set of identified but non opaque structures in the composite module.
> -    DenseSet<StructType *, StructTypeKeyInfo> NonOpaqueStructTypes;
> -
> -  public:
> -    void addNonOpaque(StructType *Ty);
> -    void switchToNonOpaque(StructType *Ty);
> -    void addOpaque(StructType *Ty);
> -    StructType *findNonOpaque(ArrayRef<Type *> ETypes, bool IsPacked);
> -    bool hasType(StructType *Ty);
> -  };
> -
> +    // The set of identified but non opaque structures in the composite module.
> +    DenseSet<StructType *, StructTypeKeyInfo> NonOpaqueStructTypes;
> +
> +    // Map between structure type name and instance. Used in findNonOpaque
> +    // to correctly map imported global variable type during ThinLTO import
> +    // phase.
> +    DenseMap<StringRef, StructType *> NonOpaqueStructNameMap;
> +
> +  public:
> +    void addNonOpaque(StructType *Ty);
> +    void switchToNonOpaque(StructType *Ty);
> +    void addOpaque(StructType *Ty);
> +    StructType *findNonOpaque(ArrayRef<Type *> ETypes, bool IsPacked,
> +                              StringRef Name);
> +    bool hasType(StructType *Ty);
> +  };
> +
>    IRMover(Module &M);
>  
>    typedef std::function<void(GlobalValue &)> ValueAdder;
> Index: llvm/trunk/test/Linker/struct-mapping.ll
> ===================================================================
> --- llvm/trunk/test/Linker/struct-mapping.ll
> +++ llvm/trunk/test/Linker/struct-mapping.ll
> @@ -0,0 +1,12 @@
> +; RUN: llvm-link --initial-module=%s %p/Inputs/struct-mapping.ll -S -o - | FileCheck %s
> +
> +; Here we check that new type mapping algorithm correctly mapped type of internal
> +; member of struct.Baz to struct.Foo. Without it we'd map that type to struct.Bar, because
> +; it is recursively isomorphic to struct.Foo and is defined first in source file.
> +; CHECK: %struct.Baz = type { i64, i64, %struct.Foo }
> +
> +%struct.Bar = type { i64, i64 }
> +%struct.Foo = type { i64, i64 }
> +
> + at bar = global %struct.Bar zeroinitializer
> + at foo = global %struct.Foo zeroinitializer
> Index: llvm/trunk/test/Linker/Inputs/struct-mapping.ll
> ===================================================================
> --- llvm/trunk/test/Linker/Inputs/struct-mapping.ll
> +++ llvm/trunk/test/Linker/Inputs/struct-mapping.ll
> @@ -0,0 +1,4 @@
> +%struct.Baz = type { i64, i64, %struct.Foo }
> +%struct.Foo = type { i64, i64 }
> +
> + at baz = global %struct.Baz zeroinitializer
> Index: llvm/trunk/lib/Linker/IRMover.cpp
> ===================================================================
> --- llvm/trunk/lib/Linker/IRMover.cpp
> +++ llvm/trunk/lib/Linker/IRMover.cpp
> @@ -318,8 +318,8 @@
>        return *Entry = Ty;
>      }
>  
> -    if (StructType *OldT =
> -            DstStructTypesSet.findNonOpaque(ElementTypes, IsPacked)) {
> +    if (StructType *OldT = DstStructTypesSet.findNonOpaque(
> +            ElementTypes, IsPacked, STy->getName())) {
>        STy->setName("");
>        return *Entry = OldT;
>      }
> @@ -906,7 +906,6 @@
>  Expected<Constant *> IRLinker::linkGlobalValueProto(GlobalValue *SGV,
>                                                      bool ForAlias) {
>    GlobalValue *DGV = getLinkedToGlobal(SGV);
> -
>    bool ShouldLink = shouldLink(DGV, *SGV);
>  
>    // just missing from map
> @@ -1410,12 +1409,14 @@
>  
>  void IRMover::IdentifiedStructTypeSet::addNonOpaque(StructType *Ty) {
>    assert(!Ty->isOpaque());
> +  if (Ty->hasName())
> +    NonOpaqueStructNameMap.insert({getTypeNamePrefix(Ty->getName()), Ty});
> +
>    NonOpaqueStructTypes.insert(Ty);
>  }
>  
>  void IRMover::IdentifiedStructTypeSet::switchToNonOpaque(StructType *Ty) {
> -  assert(!Ty->isOpaque());
> -  NonOpaqueStructTypes.insert(Ty);
> +  addNonOpaque(Ty);
>    bool Removed = OpaqueStructTypes.erase(Ty);
>    (void)Removed;
>    assert(Removed);
> @@ -1428,10 +1429,16 @@
>  
>  StructType *
>  IRMover::IdentifiedStructTypeSet::findNonOpaque(ArrayRef<Type *> ETypes,
> -                                                bool IsPacked) {
> +                                                bool IsPacked, StringRef Name) {
>    IRMover::StructTypeKeyInfo::KeyTy Key(ETypes, IsPacked);
>    auto I = NonOpaqueStructTypes.find_as(Key);
> -  return I == NonOpaqueStructTypes.end() ? nullptr : *I;
> +  if (I == NonOpaqueStructTypes.end())
> +    return nullptr;
> +  auto NI = NonOpaqueStructNameMap.find(getTypeNamePrefix(Name));
> +  if (NI != NonOpaqueStructNameMap.end() &&
> +      IRMover::StructTypeKeyInfo::KeyTy((*NI).second) == Key)
> +    return (*NI).second;
> +  return *I;
>  }
>  
>  bool IRMover::IdentifiedStructTypeSet::hasType(StructType *Ty) {
> Index: llvm/trunk/tools/llvm-link/llvm-link.cpp
> ===================================================================
> --- llvm/trunk/tools/llvm-link/llvm-link.cpp
> +++ llvm/trunk/tools/llvm-link/llvm-link.cpp
> @@ -70,6 +70,11 @@
>  OutputFilename("o", cl::desc("Override output filename"), cl::init("-"),
>                 cl::value_desc("filename"));
>  
> +static cl::opt<std::string>
> +    InitialModule("initial-module",
> +                  cl::desc("Link to existing destination module"), cl::init(""),
> +                  cl::value_desc("filename"));
> +
>  static cl::opt<bool>
>  Internalize("internalize", cl::desc("Internalize linked symbols"));
>  
> @@ -360,7 +365,9 @@
>    if (!DisableDITypeMap)
>      Context.enableDebugTypeODRUniquing();
>  
> -  auto Composite = make_unique<Module>("llvm-link", Context);
> +  auto Composite = InitialModule.empty()
> +                       ? make_unique<Module>("llvm-link", Context)
> +                       : loadFile(argv[0], InitialModule, Context);
>    Linker L(*Composite);
>  
>    unsigned Flags = Linker::Flags::None;