[PATCH] D31963: ThinLTOBitcodeWriter: keep comdats together, rename if leader is renamed

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:319
 
+  auto ShouldLiveInMergedM = [&](const GlobalValue *GV) -> bool {
+    if (auto *F = dyn_cast<Function>(GV))
----------------
I don't think you need to extract this lambda. The appropriate test is just whether the GlobalValue is a GlobalVariable with type metadata. The canonical definitions of any virtual functions identified by this lambda live in the thin LTO module, and the copies in the merged module are used for optimization only and are not comdat members (see line 347 below). So your code for identifying comdats could be added to the loop above.


================
Comment at: lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:366
 
-  promoteInternals(*MergedM, M, ModuleId);
-  promoteInternals(M, *MergedM, ModuleId);
+  std::unordered_map<std::string, std::string> ComdatsToRename;
+  promoteInternals(*MergedM, M, ModuleId, ComdatsToRename);
----------------
The code above should guarantee that if a module contains one member of a comdat it will also contain all other members. So I think you can now apply comdat renaming in `promoteInternals`.


================
Comment at: test/Transforms/ThinLTOBitcodeWriter/comdat.ll:5-26
+; MERGED: ${{"?lwt[^ ]+}} = comdat any
+; MERGED: $nlwt = comdat any
+; MERGED: {{@"?lwt_aliasee[^ ]*}} = private unnamed_addr global
+; MERGED-SAME: comdat(${{"?lwt[^ ]+}})
+; MERGED: {{@"?lwt_nl[^ ]+}} = hidden unnamed_addr global
+; MERGED-SAME: comdat(${{"?lwt[^ ]+}})
+; MERGED: {{@"?nlwt_aliasee[^ ]*}} = private unnamed_addr global
----------------
Please move these checks next to the corresponding input IR (where possible).


https://reviews.llvm.org/D31963