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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:694
 
+  typedef ModulePathStringTableTy::value_type Module;
+
----------------
tejohnson wrote:
> The patch adds a couple different new types named Module - is it possible to use something else rather than overloading that name?
I will rename the other one to `AddedModule`.


================
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,
----------------
tejohnson wrote:
> Update description of return value.
Will do


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


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:724
+  /// if it is expecting to read a per-module summary index.
+  ModuleSummaryIndex::Module *Mod = nullptr;
+
----------------
tejohnson wrote:
> 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?
I think I originally wanted this to work by changing addModule so that it would always either create a module or error out rather than possibly returning an existing one, and then have the client always create the module on its own (so that it could control whether or not to create a module for each input file, and as a side effect we would get the optimization that you mention), but then I thought that that wouldn't necessarily work because the pointer could become invalidated by the StringMap (although now that I look at StringMap's rehash implementation I don't think that could actually happen). So I ended up moving the Module creation back into the reader.

I'd be fine with changing this back to be more like how it was and we can think about changing the API at a later point.

Regarding the comment, I mean that the caller (i.e. `readSummary`) will fill this field in as it is expecting to read a per-module summary index. I'll make this comment clearer if I make this change in a followup.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:4695
+  if (!Mod)
+    Mod = TheIndex.addModule(ModulePath, 0);
+  return Mod;
----------------
tejohnson wrote:
> 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?
Yes, if `!Mod` at this point we must have been called from `getSummary` for which the current behaviour is that the module id will be 0 if it is a per-module summary.

It's more of a pure optimization at this point, so I'll revert it as I mentioned on the other comment.


================
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();
----------------
tejohnson wrote:
> 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.
I did it this way because I wanted LTO to handle regular LTO module summaries on its own (well, as much as possible I suppose). With that suggestion, we would move more of the handling into the bitcode reader, which I'm not entirely opposed to, but let me know what you think of that rationale.


================
Comment at: llvm/lib/LTO/LTO.cpp:509
+  } else {
+    return linkRegularLTO(std::move(*ModOrErr), /*LivenessFromIndex=*/false);
+  }
----------------
tejohnson wrote:
> Why not just link all of the regular LTO objects during runRegularLTO? I guess you still need to keep track of which had summaries though. How much of addRegularLTO could then be deferred until runRegularLTO (i.e. keeping track of the resolution info, like we do for ThinLTO, but delaying building the Module, computing Keep, etc)? It seems conceptually simpler to do all the linking in one place.
I think the first thing I tried was something along those lines, but the code turned out to be less clean in my opinion, precisely because there was more state to keep track of. I guess it's just a matter of opinion, but this seems cleaner to me, as there turned out to be a relatively clean boundary between preparing to link a module and linking it.


================
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);
----------------
tejohnson wrote:
> 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.
Will do


https://reviews.llvm.org/D33922