[PATCH] D27324: IPO: Introduce ThinLTOBitcodeWriter pass.

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:73
+
+// Export each local-linkage entity defined by ExportM and used by ImportM by
+// changing visibility and appending the given ModuleId.
----------------
We call this export "Promotion" in ThinLTO usually I believe.


================
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
----------------
This seems dead code to me, the returned value is ` return ("$" + Str).str();` which can't be empty.


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:210
+
+  auto IsInRegularM = [&](const GlobalValue *GV) {
+    auto *GVar = dyn_cast<GlobalVariable>(GV->getBaseObject());
----------------
"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.


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:211
+  auto IsInRegularM = [&](const GlobalValue *GV) {
+    auto *GVar = dyn_cast<GlobalVariable>(GV->getBaseObject());
+    if (!GVar)
----------------
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?


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:217
+    GVar->getMetadata(LLVMContext::MD_type, MDs);
+    return !MDs.empty();
+  };
----------------
Looks like we could have method `bool GlobalObject::hasMD(unsigned KindID);`



================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:225
+  std::unique_ptr<Module> ThinM(CloneModule(
+      &M, VMap2, [&](const GlobalValue *GV) { return !IsInRegularM(GV); }));
+
----------------
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?


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:295
+};
+}
+
----------------
`} // anonymous namespace`


https://reviews.llvm.org/D27324