[LTO] Turn ImportListsTy into a proper class (NFC)

[kazutakahirata]

This patch turns ImportListsTy into a class that wraps
DenseMap<StringRef, ImportMapTy>.

Here is the background. I'm planning to reduce the memory footprint
of ThinLTO indexing. Specifically, ImportMapTy, the list of imports
for a given destination module, will be a hash set of integer IDs
indexing into a deduplication table of pairs (SourceModule, GUID),
which is a lot like string interning. I'm planning to put this
deduplication table as part of ImportListsTy and have each instance of
ImportMapTy hold a reference to the deduplication table.

Another reason to wrap the DenseMap is that I need to intercept
operator so that I can construct an instance of ImportMapTy with a
reference to the deduplication table. Note that the default
implementation of operator would default-construct ImportMapTy,
which I am going to disable.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Kazu Hirata (kazutakahirata)

Changes

This patch turns ImportListsTy into a class that wraps
DenseMap<StringRef, ImportMapTy>.

Here is the background. I'm planning to reduce the memory footprint
of ThinLTO indexing. Specifically, ImportMapTy, the list of imports
for a given destination module, will be a hash set of integer IDs
indexing into a deduplication table of pairs (SourceModule, GUID),
which is a lot like string interning. I'm planning to put this
deduplication table as part of ImportListsTy and have each instance of
ImportMapTy hold a reference to the deduplication table.

Another reason to wrap the DenseMap is that I need to intercept
operator so that I can construct an instance of ImportMapTy with a
reference to the deduplication table. Note that the default
implementation of operator would default-construct ImportMapTy,
which I am going to disable.

---

Full diff: https://github.com/llvm/llvm-project/pull/106427.diff

1 Files Affected:

• (modified) llvm/include/llvm/Transforms/IPO/FunctionImport.h (+18-1)

diff --git a/llvm/include/llvm/Transforms/IPO/FunctionImport.h b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
index c06d96bbe62e22..78932c12e76ff8 100644
--- a/llvm/include/llvm/Transforms/IPO/FunctionImport.h
+++ b/llvm/include/llvm/Transforms/IPO/FunctionImport.h
@@ -150,7 +150,24 @@ class FunctionImporter {
   };
 
   // A map from destination modules to lists of imports.
-  using ImportListsTy = DenseMap<StringRef, ImportMapTy>;
+  class ImportListsTy {
+  public:
+    ImportListsTy() = default;
+    ImportListsTy(size_t Size) : ListsImpl(Size) {}
+
+    ImportMapTy &operator[](StringRef DestMod) {
+      return ListsImpl.try_emplace(DestMod).first->second;
+    }
+
+    size_t size() const { return ListsImpl.size(); }
+
+    using const_iterator = DenseMap<StringRef, ImportMapTy>::const_iterator;
+    const_iterator begin() const { return ListsImpl.begin(); }
+    const_iterator end() const { return ListsImpl.end(); }
+
+  private:
+    DenseMap<StringRef, ImportMapTy> ListsImpl;
+  };
 
   /// The set contains an entry for every global value that the module exports.
   /// Depending on the user context, this container is allowed to contain

[nikic]

We've encountered an issue when integrating this change into Rust. The problem is that this commit removed the possibility to fetch the import list without potentially modifying the DenseMap. Rust can read the ImportLists concurrently from multiple threads, and this may result in crashes, as reported at rust-lang/rust#129749 (comment). At least that's my current theory.

Rust was previously using lookup() instead of operator[]. Would it be possible to restore some form of read-only access to this data structure, whether via lookup or some other method?

[kazutakahirata]

We've encountered an issue when integrating this change into Rust. The problem is that this commit removed the possibility to fetch the import list without potentially modifying the DenseMap. Rust can read the ImportLists concurrently from multiple threads, and this may result in crashes, as reported at rust-lang/rust#129749 (comment). At least that's my current theory.

Rust was previously using lookup() instead of operator[]. Would it be possible to restore some form of read-only access to this data structure, whether via lookup or some other method?

So, are there two problems here?

• Preserving const for the import list if we are accessing it in a read-only manner
• Crashes from concurrent accesses

I'm happy to switch to lookup or find to preserve const, but I don't see how that fixes crashes. In LLVM, we build the import lists for different destination modules in a single thread and then we do actual imports in parallel. In Rust, do you do something a bit different like building import lists in parallel?

[nikic]

I'm happy to switch to lookup or find to preserve const, but I don't see how that fixes crashes. In LLVM, we build the import lists for different destination modules in a single thread and then we do actual imports in parallel. In Rust, do you do something a bit different like building import lists in parallel?

It's the same in Rust. It's just that previously the part doing the concurrent accesses was using read-only accesses, while it's now doing potential read-write accesses. From an API perspective this is definitely wrong.

Of course, it should only actually be an issue if one of the module identifiers is not part of ImportLists when doing the actual imports, so maybe something is wrong on the Rust side...

[kazutakahirata]

I'm happy to switch to lookup or find to preserve const, but I don't see how that fixes crashes. In LLVM, we build the import lists for different destination modules in a single thread and then we do actual imports in parallel. In Rust, do you do something a bit different like building import lists in parallel?

It's the same in Rust. It's just that previously the part doing the concurrent accesses was using read-only accesses, while it's now doing potential read-write accesses. From an API perspective this is definitely wrong.

Of course, it should only actually be an issue if one of the module identifiers is not part of ImportLists when doing the actual imports, so maybe something is wrong on the Rust side...

I see. I guess it's still handy to have a read-only access to ImportLits. I've posted #109036. Please take a look. Thanks!

[nikic]

After some further investigation, this issue occurs when empty or mostly empty (e.g. module inline asm only) modules are part of the thin link.

And the reason why LLVM did not hit this problem is this code:

llvm-project/llvm/lib/LTO/ThinLTOCodeGenerator.cpp

Lines 1102 to 1111 in 0dd5685

	// Make sure that every module has an entry in the ExportLists, ImportList,
	// GVSummary and ResolvedODR maps to enable threaded access to these maps
	// below.
	for (auto &Module : Modules) {
	auto ModuleIdentifier = Module->getName();
	ExportLists[ModuleIdentifier];
	ImportLists[ModuleIdentifier];
	ResolvedODR[ModuleIdentifier];
	ModuleToDefinedGVSummaries[ModuleIdentifier];
	}

So LLVM's solution here was to force-initialize all map entries, while Rust's solution was to use read-only access to the maps.

[teresajohnson]

After some further investigation, this issue occurs when empty or mostly empty (e.g. module inline asm only) modules are part of the thin link.

And the reason why LLVM did not hit this problem is this code:

llvm-project/llvm/lib/LTO/ThinLTOCodeGenerator.cpp

Lines 1102 to 1111 in 0dd5685

	// Make sure that every module has an entry in the ExportLists, ImportList,
	// GVSummary and ResolvedODR maps to enable threaded access to these maps
	// below.
	for (auto &Module : Modules) {
	auto ModuleIdentifier = Module->getName();
	ExportLists[ModuleIdentifier];
	ImportLists[ModuleIdentifier];
	ResolvedODR[ModuleIdentifier];
	ModuleToDefinedGVSummaries[ModuleIdentifier];
	}

That is the old LTO API but fyi the new LTO API (used by lld and gold) uses the same approach:

llvm-project/llvm/lib/LTO/LTO.cpp

Lines 1706 to 1715 in 0dd5685

	// Create entries for any modules that didn't have any GV summaries
	// (either they didn't have any GVs to start with, or we suppressed
	// generation of the summaries because they e.g. had inline assembly
	// uses that couldn't be promoted/renamed on export). This is so
	// InProcessThinBackend::start can still launch a backend thread, which
	// is passed the map of summaries for the module, without any special
	// handling for this case.
	for (auto &Mod : ThinLTO.ModuleMap)
	if (!ModuleToDefinedGVSummaries.count(Mod.first))
	ModuleToDefinedGVSummaries.try_emplace(Mod.first);

So LLVM's solution here was to force-initialize all map entries, while Rust's solution was to use read-only access to the maps.