[PATCH] D43077: [ThinLTO] Import external globals

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: lib/Linker/IRMover.cpp:1442
+      IRMover::StructTypeKeyInfo::KeyTy((*NI).second) == Key)
+    return (*NI).second;
+  return *I;
----------------
evgeny777 wrote:
> 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
> 
Thanks for the detailed explanation. Since this isn't specific to the importing of variables, and can happen already with functions, please do separate out into a different patch.


https://reviews.llvm.org/D43077