[PATCH] D43077: [ThinLTO] Import external globals

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 added inline comments.


================
Comment at: lib/Linker/IRMover.cpp:1442
+      IRMover::StructTypeKeyInfo::KeyTy((*NI).second) == Key)
+    return (*NI).second;
+  return *I;
----------------
tejohnson wrote:
> I'm not as familiar with the struct mapping - what happens without this change? Why did importing variables necessitate it?
Consider this C++ program, which consists of three TUs:

main.cpp:
```
struct Bar { i64 a,b; } B; // Do not optimize this out
struct Foo { i64 a, b; }; // Foo is isomorphic to Bar

Foo *getFoo(); // We'll import this function from foo.cpp
struct S { Foo *_f } instance = { getFoo(); };

void main() { return instance._f->a; }
```

foo.cpp
```
struct Foo { i64 a, b; }; // Same as in main.cpp
struct Baz { i64 a, b; Foo f; }; 

Foo *gFoo;
extern Baz *gBaz;

Foo *getFoo() {
  return gFoo ? gFoo : &gBaz->f;
}
```

baz.cpp
```
struct Foo { i64 a, b; };
struct Baz { i64 a, b; Foo f; };  // Same as in foo.cpp

Baz *gBaz;
```

Now you perform thinlto import and get following definition of Baz in main.o0.3.import.bc when no name mapping used:
```
%struct.Bar = type { i64, i64 }
%struct.Foo = type { i64, i64 }

; This is incorrect. We should have %struct.Foo in the 3rd field
; This happens because Foo and Bar are recursively isomorphic
; and Bar is seen first by IRLinker.
%struct.Baz = type { i64, i64, %struct.Bar }  
```

The faulted import causes bitcast created in static constructor:
```
; This static constructor can't be evaluated  due to a bitcast (Bar -> Foo)
define internal void @_GLOBAL__sub_I_main.cpp() #1 section ".text.startup" {
entry:
  %call.i.i = tail call %struct.Foo* bitcast (%struct.Bar* ()* @_Z6getFoov to %struct.Foo* ()*)()
  store %struct.Foo* %call.i.i, %struct.Foo** getelementptr inbounds (%struct.S, %struct.S* @instance, i64 0, i32 0), align 8, !tbaa !2
  ret void
}
```

Adding name mapping fixes the problem:
```
%struct.Baz = type { i64, i64, %struct.Foo } ; Good

; No bitcast - evaluator is happy.
define internal void @_GLOBAL__sub_I_main.cpp() section ".text.startup" {
entry:
  %call.i.i = tail call %struct.Foo* @getFoo()
  store %struct.Foo* %call.i.i, %struct.Foo** getelementptr inbounds (%struct.S, %struct.S* @instance, i64 0, i32 0), align 8
  ret void
}
```

The problem isn't specific at all to importing globals. You can easily rewrite this example to using functions (i.e gBaz -> getBaz) and get the same result. (it probably makes sense to divide this patch into two separate ones).

BTW the negative impact of those bitcasts isn't limited to evaluator. It also affects inliner behavior - see D38896



https://reviews.llvm.org/D43077