[PATCH] D70128: [ThinLTO] Add correctness check for RO/WO variable import

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 created this revision.
evgeny777 added a reviewer: tejohnson.
Herald added subscribers: dexonsmith, hiraditya, inglorion.
Herald added a project: LLVM.

This was suggested in D70006 <https://reviews.llvm.org/D70006>. The check is a bit expensive so is put under NDEBUG


https://reviews.llvm.org/D70128

Files:
  llvm/lib/Transforms/IPO/FunctionImport.cpp
  llvm/lib/Transforms/Utils/FunctionImportUtils.cpp


Index: llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
===================================================================
--- llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
+++ llvm/lib/Transforms/Utils/FunctionImportUtils.cpp
@@ -250,12 +250,12 @@
       // contains summaries from the source modules if they are being imported.
       // We might have a non-null VI and get here even in that case if the name
       // matches one in this module (e.g. weak or appending linkage).
-      auto* GVS = dyn_cast_or_null<GlobalVarSummary>(
-          ImportIndex.findSummaryInModule(VI, M.getModuleIdentifier()));      
+      auto *GVS = dyn_cast_or_null<GlobalVarSummary>(
+          ImportIndex.findSummaryInModule(VI, M.getModuleIdentifier()));
       if (GVS &&
           (ImportIndex.isReadOnly(GVS) || ImportIndex.isWriteOnly(GVS))) {
         V->addAttribute("thinlto-internalize");
-        // Objects referenced by writeonly GV initializer should not be 
+        // Objects referenced by writeonly GV initializer should not be
         // promoted, because there is no any kind of read access to them
         // on behalf of this writeonly GV. To avoid promotion we convert
         // GV initializer to 'zeroinitializer'. This effectively drops
Index: llvm/lib/Transforms/IPO/FunctionImport.cpp
===================================================================
--- llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -627,6 +627,35 @@
 }
 #endif
 
+static bool
+checkVariableImport(const ModuleSummaryIndex &Index,
+                    StringMap<FunctionImporter::ImportMapTy> &ImportLists,
+                    StringMap<FunctionImporter::ExportSetTy> &ExportLists) {
+
+  DenseSet<GlobalValue::GUID> ReadOrWriteOnlyVars;
+  auto IsReadOrWriteOnlyVar = [&](StringRef ModuleId, GlobalValue::GUID G) {
+    auto *GVS = dyn_cast_or_null<GlobalVarSummary>(
+        Index.findSummaryInModule(G, ModuleId));
+    return GVS && (Index.isReadOnly(GVS) || Index.isWriteOnly(GVS));
+  };
+
+  for (auto &ExportPerModule : ExportLists)
+    for (auto GUID : ExportPerModule.second)
+      if (IsReadOrWriteOnlyVar(ExportPerModule.first(), GUID))
+        ReadOrWriteOnlyVars.insert(GUID);
+
+  // Checks that all GUIDs of read/writeonly vars we see in export lists
+  // are also in the import lists. Otherwise we my face linker undefs,
+  // because readonly and writeonly vars are internalized in their
+  // source modules.
+  for (auto GUID : ReadOrWriteOnlyVars)
+    for (auto &ImportPerModule : ImportLists)
+      for (auto &ExportPerModule : ImportPerModule.second)
+        if (ExportPerModule.second.find(GUID) == ExportPerModule.second.end())
+          return false;
+  return true;
+}
+
 /// Compute all the import and export for every module using the Index.
 void llvm::ComputeCrossModuleImport(
     const ModuleSummaryIndex &Index,
@@ -660,6 +689,7 @@
   }
 
 #ifndef NDEBUG
+  assert(checkVariableImport(Index, ImportLists, ExportLists));
   LLVM_DEBUG(dbgs() << "Import/Export lists for " << ImportLists.size()
                     << " modules:\n");
   for (auto &ModuleImports : ImportLists) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D70128.228901.patch
Type: text/x-patch
Size: 3167 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191112/cbfa4690/attachment.bin>