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

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

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.


================
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
----------------
`s/function/functions/`

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:366
@@ -365,3 @@
-  // symbols referenced from asm
-  UpdateCompilerUsed(TheModule, TM, AsmUndefinedRefs);
-
----------------
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.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:793
@@ -879,1 +792,3 @@
+                                     recordNewLinkage);
+
   // Parallel optimizer + codegen
----------------
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?

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:536
@@ +535,3 @@
+
+  llvm::internalizeModule(TheModule, MustPreserveGV);
+}
----------------
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?


http://reviews.llvm.org/D20290