IRMover: remove unused (?) code

[evgeny777]

Summary

I've come across this strange piece of code while trying to teach ThinLTO to import external constant objects. It looks like no test is dependent on it. What's the purpose of it?

[tejohnson]

I'm not terribly familiar with this aspect of IRMover. Hoping Rafael can comment. I did find the commit where Rafael added it, here is the commit log message:

---

commit 7a551b7c6dd012d67ddf27ab8d87c3e8742c5f11
Author: Rafael Espindola <[e-mail]>
Date: Mon Dec 1 04:15:59 2014 +0000

Change how we keep track of which types are in the dest module.

Instead of keeping an explicit set, just drop the names of types we choose
to map to some other type.

This has the advantage that the name of the unused will not cause the context
to rename types on module read.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@222986 91177308-0d34-0410-b5e6-96231b3b80d8

include/llvm/Linker/Linker.h
lib/Linker/LinkModules.cpp

test/Linker/type-unique-dst-types.ll

The change to type-unique-dst-types.ll got rid of this line:

-; CHECK-NEXT: %A.11.1 = type opaque

and added:
+; CHECK: @g3 = external global %A
+; CHECK: @g1 = external global %A
+; CHECK: @g2 = external global %A

It's possible that the change to this test wasn't sufficient to make it fail without the patch (the change to the test didn't look for the absence of %A.11.1). From my reading of the commit message, this partly an optimization to avoid unnecessarily renaming some structures due to matching names in Src and Dst. Note that the CHECK lines added above would also match against say:
+; CHECK: @g3 = external global %A.11.1

Maybe just look at the raw assembly dump from llvm-link in this test and see if it differs with and without the patch.

[evgeny777]

Yes, I've seen that as well, but it seems to me that type mapping logic has changed since then. @rafael ?

[evgeny777]

Ok, I found an answer. Calling this off.

[tejohnson]

In D42418#986080, @evgeny777 wrote:

Ok, I found an answer. Calling this off.

Great. Can you add a comment to this block of code explaining why it is needed? Thanks!

lib/Linker/IRMover.cpp

if (!areTypesIsomorphic(DstTy, SrcTy)) {
     // any speculative mappings we've established.
     for (Type *Ty : SpeculativeTypes)
       MappedTypes.erase(Ty);
 
     SrcDefinitionsToResolve.resize(SrcDefinitionsToResolve.size() -
                                    SpeculativeDstOpaqueTypes.size());
     for (StructType *Ty : SpeculativeDstOpaqueTypes)
       DstResolvedOpaqueTypes.erase(Ty);
-  } else {
-    for (Type *Ty : SpeculativeTypes)
-      if (auto *STy = dyn_cast<StructType>(Ty))
-        if (STy->hasName())
-          STy->setName("");
   }
   SpeculativeTypes.clear();
   SpeculativeDstOpaqueTypes.clear();
 }
 
 /// Recursively walk this pair of types, returning true if they are isomorphic,
 /// false if they are not.
 bool TypeMapTy::areTypesIsomorphic(Type *DstTy, Type *SrcTy) {