[PATCH] D33922: Apply summary-based dead stripping to regular LTO modules with summaries.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:694
 
+  typedef ModulePathStringTableTy::value_type Module;
+
----------------
The patch adds a couple different new types named Module - is it possible to use something else rather than overloading that name?


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:697
   /// Add a new module path with the given \p Hash, mapped to the given \p
   /// ModID, and return an iterator to the entry in the index.
+  Module *addModule(StringRef ModPath, uint64_t ModId,
----------------
Update description of return value.


================
Comment at: llvm/include/llvm/LTO/LTO.h:287
+    // that we can link it later, after dead stripping.
+    struct Module;
+    std::vector<Module> ModsWithSummaries;
----------------
Can you define this structure here in the header? I think it would be clearer.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:724
+  /// if it is expecting to read a per-module summary index.
+  ModuleSummaryIndex::Module *Mod = nullptr;
+
----------------
I'm a bit confused about the relationship between ModulePath and Mod. It seems from the code in addThisModule() that Mod is being used to save on insertion attempts into the table, or is there another reason? If just an optimization, can that be committed separately?

I don't quite understand the last part of the comment "if it is expecting to read a per-module summary index".  Do you mean it is only filled in when we are (in the process of) reading a per-module summary index?


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:4695
+  if (!Mod)
+    Mod = TheIndex.addModule(ModulePath, 0);
+  return Mod;
----------------
I take it we were always guaranteed to have ModuleId == 0 when we called addThisModulePath?

Also, previously we would repeatedly try to re-add the module path - is saving the result of addModule in the reader and checking it here just an optimization, or required because of the other changes?


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:5509
+  ModuleSummaryIndexBitcodeReader R(std::move(Stream), Strtab, CombinedIndex);
+  R.Mod = CombinedIndex.addModule(ModulePath, ModuleId);
   return R.parseModule();
----------------
It wasn't clear to me when I first looked at this part of the patch why the ModuleIdentifier on the BitcodeModule wouldn't be used, as it is in getSummary below (or was here before). I think it would be clearer to pass in a flag indicating that this is a regularLTO summary, and then here either use the ModuleIdentifier or "", depending on the flag.


================
Comment at: llvm/lib/LTO/LTO.cpp:605
+        // the symbol, so we may be able to link it with available_externally
+        // linkage.
+        Mod.Keep.push_back(GV);
----------------
I think the old comment is more accurate (we can link it as available_externally, but only need to if it is undefined). Perhaps just add that we will decide later when we link this module, based on whether it is undefined.


https://reviews.llvm.org/D33922