[PATCH] D20290: [ThinLTO] Refactor ODR resolution and internalization (NFC)

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In http://reviews.llvm.org/D20290#431538, @joker.eph wrote:

> A few first comments to begin with.
>
> Thanks for doing this, I really planned to do it and you're cleaning behind me actually, wasn't it too painful to follow what was going on? I had the impression that things went a bit out-of-control with time and I was afraid the logic would be hard to refactor.


No problem, it helps me make forward progress on some other ThinLTO improvements. Wasn't too difficult to follow. Anyway, I reviewed the code, so I had better be able to follow it. =)


================
Comment at: lib/LTO/LTO.cpp:55
@@ +54,3 @@
+//
+// We'd like to drop these function if they are no longer referenced in the
+// current module. However there is a chance that another module is still
----------------
joker.eph wrote:
> `s/function/functions/`
ok

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:366
@@ -365,3 @@
-  // symbols referenced from asm
-  UpdateCompilerUsed(TheModule, TM, AsmUndefinedRefs);
-
----------------
joker.eph wrote:
> This call disappeared?
> I can buy that you don't need it, but I rather have a first "NFC" commit that removes the call here and move the behavior in `MustPreserveGV`, and then rebase this patch on top of it so the refactoring here would be easier to follow.
We don't need it since the AsmUndefinedRefs is checked by the new MustPreserveGV callback. But the change was actually instigated because UpdateCompilerUsed is in lib/LTO, and I was moving this function to lib/Transforms/IPO, which would have caused a circular dependence.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:793
@@ -879,1 +792,3 @@
+                                     recordNewLinkage);
+
   // Parallel optimizer + codegen
----------------
joker.eph wrote:
> It seems to me that the 30 lines block above should be refactored with the block starting line 533.
> 
> It is not clear to me why you need to have this done as a global step, compared to the existing per-thread ResolveODR?
Yeah, let me refactor those together.

Needs to be a global step so that distributed builds can use the same approach, communicating the results of this analysis to backends via the index.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:536
@@ +535,3 @@
+
+  llvm::internalizeModule(TheModule, MustPreserveGV);
+}
----------------
joker.eph wrote:
> It it not clear if we still need to use the internalize util, is it temporary? What does it do right now other than iterating over the globals and calling the call-back?
Perhaps we could just set the linkage directly for internalized symbols (that's what gold-plugin does for LTO internalization). I was trying to stick closer to the mechanism being used by ThinLTOCodeGenerator.


http://reviews.llvm.org/D20290