[PATCH] D27324: IPO: Introduce ThinLTOBitcodeWriter pass.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc marked 4 inline comments as done.
pcc added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:201
+  std::string ModuleId = getModuleId(&M);
+  if (ModuleId.empty()) {
+    // We couldn't generate a module ID for this module, just write it out as a
----------------
mehdi_amini wrote:
> This seems dead code to me, the returned value is ` return ("$" + Str).str();` which can't be empty.
See line 63, also the test case unsplittable.ll.


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:210
+
+  auto IsInRegularM = [&](const GlobalValue *GV) {
+    auto *GVar = dyn_cast<GlobalVariable>(GV->getBaseObject());
----------------
mehdi_amini wrote:
> "Regular" was misleading to me, because I see the ThinLTO module as the regular one, the other that only contains the VTable and the types is the "special" one.
Renamed to MergedM.


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:211
+  auto IsInRegularM = [&](const GlobalValue *GV) {
+    auto *GVar = dyn_cast<GlobalVariable>(GV->getBaseObject());
+    if (!GVar)
----------------
mehdi_amini wrote:
> Should this be a check on GlobalObject instead? Technically GlobalObject can have MDs attached. I guess you assume that we'll never see a typeMD on anything else than a GlobalVariable?
Yes, at this point we only support type metadata on GlobalVariables. We can also have type metadata attached to Functions, which is used to support CFI on  indirect calls, but that will be implemented separately (and much differently).


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:217
+    GVar->getMetadata(LLVMContext::MD_type, MDs);
+    return !MDs.empty();
+  };
----------------
mehdi_amini wrote:
> Looks like we could have method `bool GlobalObject::hasMD(unsigned KindID);`
> 
It doesn't seem worth it to me, we're basically only doing this in this pass.


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:225
+  std::unique_ptr<Module> ThinM(CloneModule(
+      &M, VMap2, [&](const GlobalValue *GV) { return !IsInRegularM(GV); }));
+
----------------
mehdi_amini wrote:
> I'm concerned about the cost: cloning a module isn't free.
> Why do you need to clone the Thin module? Can't you just strip it from what was moved to the other?
> Could we just have an option to clone module so that it actually move directly so that it'll do exactly what's needed?
I added a filterModule function that operates on an existing module and used it here.


https://reviews.llvm.org/D27324