[PATCH] D17066: libLTO: add a ThinLTOCodeGenerator on the model of LTOCodeGenerator.

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Some misc comments below.


================
Comment at: include/llvm-c/lto.h:666
@@ +665,3 @@
+/**
+ * Sets the expiration (in seconds) for an entry in the cache.
+ *
----------------
What is the default?

================
Comment at: include/llvm/LTO/ThinLTOCodeGenerator.h:1
@@ +1,2 @@
+//===-LTOCodeGenerator.h - LLVM Link Time Optimizer -----------------------===//
+//
----------------
Fix file name.

================
Comment at: include/llvm/LTO/ThinLTOCodeGenerator.h:46
@@ +45,3 @@
+
+/// Cache behavior controls.
+struct CachingOptions {
----------------
How do PruningInterval and Expiration interact? Are they mutually exclusive, or does a module get pruned at the min of these?

================
Comment at: include/llvm/LTO/ThinLTOCodeGenerator.h:99
@@ +98,3 @@
+  /**
+   * \defgroup Cache controling options
+   * @{
----------------
s/controling/controlling/

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:65
@@ +64,3 @@
+        getLazyBitcodeModule(MemoryBuffer::getMemBuffer(Buffer, false), Context,
+                             /* ShouldLazyLoadMetadata */ false);
+  } else {
----------------
Why not lazy load metadata too?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:84
@@ +83,3 @@
+  // User asked to save temps, let dump the bitcode file after import.
+  auto SaveTempPath = TempDir + llvm::utostr(count) + Suffix;
+  std::error_code EC;
----------------
Include Module's SourceFileName to get more meaningful dump file names?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:96
@@ +95,3 @@
+/// \return true if the global will be internalized.
+static bool InternalizeGlobal(Module &TheModule, GlobalValue &GV,
+                              StringSet<> &Keep, const FunctionInfoIndex &Index,
----------------
This is doing more than internalization, so name should probably be adjusted to reflect that. I don't have a good name off the top of my head. Maybe split into 2 and call the first something like resolveLinkOnceAndWeak, and if that returns false then invoke the internalization handling (maybe outline  keepIfPreserved and make it return a bool).

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:104
@@ +103,3 @@
+
+  if (GV.hasAvailableExternallyLinkage()) {
+    Keep.insert(GV.getName());
----------------
Should available_externally also be kept in SingleModule mode?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:112
@@ +111,3 @@
+
+  // There is nothing more restrictive than private linkage.
+  if (GV.hasPrivateLinkage())
----------------
I'm confused by the comment. It is already local if private, so no need to internalize or add to Keep, which is consistent with what the code is doing. I just don't understand what the comment about being restrictive is saying.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:160
@@ +159,3 @@
+  }
+  if (Linkage == GlobalValue::LinkOnceODRLinkage) {
+    // We need to emit only one of these, the first module will keep
----------------
Combine the LinkOnce cases and just pick the appropriate Weak linkage based on the LinkOnce linkage, the handling is identical otherwise.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:180
@@ +179,3 @@
+  }
+  if (Linkage == GlobalValue::WeakODRLinkage) {
+    // We need to emit only one of these, the first module will keep
----------------
Combine the Weak* cases, the handling is identical.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:211
@@ +210,3 @@
+// Helper function running internalize for ThinLTO
+static void InternalizeModule(Module &TheModule, const FunctionInfoIndex &Index,
+                              const StringSet<> &PreservedSymbols,
----------------
This is doing more than internalization. How about a more generic name such as LinkageOptimization or something like that?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:271
@@ +270,3 @@
+  {
+    // Run the internalize pass and some trivial optimizations
+    legacy::PassManager PM;
----------------
This handling is already outlined in the other InternalizeModule() function, invoke here.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:317
@@ +316,3 @@
+      // optimizations already ran, we have a pre-optimized copy.
+      return loadModuleFromBuffer(OptimizedBuffer->getMemBufferRef(), Ctx,
+                                  Lazy);
----------------
Does this need to be guarded by lock? Comments for LockM indicate it is just to guard optimization.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:329
@@ +328,3 @@
+    if (!PreservedSymbols.empty() || !CrossReferencedSymbols.empty())
+      InternalizeModule(*TheModule, Index, PreservedSymbols,
+                        CrossReferencedSymbols, SingleModule);
----------------
Why don't we want to do this when there are no preserved or cross referenced symbols? Couldn't the internalization be even more aggressive in that case? Also, wouldn't we at least want the linkonce/weak handling?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:343
@@ +342,3 @@
+
+class PreOptimizedModuleMap {
+  /// Helper map to find the original Module buffer from its identifier
----------------
Needs doxygen class description. Is "EarlyOptimizedModuleMap" more accurate though?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:345
@@ +344,3 @@
+  /// Helper map to find the original Module buffer from its identifier
+  /// !! Relies on having a unique getBufferIdentifier() !!
+  StringMap<MemoryBufferRef> ModuleMap;
----------------
When would this not be the case?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:352
@@ +351,3 @@
+  /// Map of Module Identifier to MemoryBuffer used to load a Module.
+  std::unordered_map<std::string, EarlyOptimizedModule> OptimizedModuleMap;
+
----------------
Out of curiosity, why a StringMap for ModuleMap and an unordered_map from string here? They are both index by the identifier string.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:410
@@ +409,3 @@
+
+static void crossImportForModule(Module &TheModule,
+                                 const FunctionInfoIndex &Index,
----------------
crossImportIntoModule?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:592
@@ +591,3 @@
+  /// Helper to load a cached module after early global optimizations
+  PreOptimizedModuleMap ModuleMap(Modules, Index, PreservedSymbols,
+                                  CrossReferencedSymbols);
----------------
Doesn't instantiating here mean that the map isn't leveraged across threads? Oh, I see that this routine and the above promote() are only invoked from llvm-lto. Is there a reason why llvm-lto doesn't use ThinLTOCodeGenerator::run()?


http://reviews.llvm.org/D17066