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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Thanks for doing this. I have some ideas on how to reduce the overhead of this checking below.



================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:653
+    for (auto &ImportPerModule : ImportLists)
+      for (auto &ExportPerModule : ImportPerModule.second)
+        if (ExportPerModule.second.find(GUID) == ExportPerModule.second.end())
----------------
There's presumably a number of duplicates across the various per module import lists, so I'm thinking it would be more efficient to flatten these sets first into a single set of imported GUIDs. Also, it should be more efficient to change the algorithm here to do something like:

Flatten ImportLists -> FlattenedImports
Change loop that walks Export lists to first check if each exported GUID is in the Flattened Imports, and only if not check if it IsReadOrWriteOnlyVar.

The advantage of this is that this avoids doing a hash table lookup for each exported GUID (looking it up in the index), so that we only do so for those that are potentially problematic (because they are not imported anywhere).

An alternative would be to compute the ReadOrWriteOnlyVars exported GUID set as the export lists are built during importing, but only under !NDEBUG. I'm not sure if that is necessary though after the restructuring I suggest above, which should hopefully streamline this so that it is reasonable in debug builds.

This also makes me think it might be a good idea to consider changing the ExportLists to hold the ValueInfos rather than GUIDs (not suggesting you do that here, more a note to self for future). After poking around at the uses of the export lists it seems very doable, and would avoid the need for index hash table lookups to locate the associated summaries here and anywhere else in the future...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70128/new/

https://reviews.llvm.org/D70128