From bc370a771d5a2d066a9704f0e9f10b9b015b115a Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Mon, 20 May 2024 18:02:25 +0200 Subject: [PATCH] Avoid relying on a hashtable iteration order. (#3960) The toolchain was iterating a map of package name to import list in a couple of places to do things beyond counting or other order-invariant operations. The result was that the specific hashtable iteration order influenced the order of SemIR generated (and other behaviors like diagnostic emission I suspect, but the source location sorting probably hid this). Switching to a hashtable implementation with any order seeding that changes run-to-run immediately shows the SemIR order fluctuating without this. This PR fixes that by instead accumulating the package imports data in a vector and using a map to vector indices. This is also slightly more efficient, although that seems unlikely to be an important factor here. There was only one insertion point so I've just hand coded the management of the indices and map, but happy to take a different approach or use an abstraction here if desired. One awkward aspect of this is that some of the loops need access to the package name as well. I've just added storage for that as these seem unlikely to be huge arrays of 10s of 1000s of imported packages, so the double storage of the identifier ID seems likely OK. But again, happy to take a different approach here if desired. It also seems like it might be possible to work out the identifier from the node, but I kept the patch more direct for simplicity. I've also not used the LLVM `MapVector` abstraction of this pattern. This was mostly to avoid adding another layer of abstractions to our data structures, and because this is the first time we've hit this really. My experience is also that it is reasonably often that there is a more efficient way to orient the vector and map than what is automatically provided. But that may just be my experience. --- toolchain/check/check.cpp | 64 +++++++++++-------- .../class/cross_package_import.carbon | 10 +-- .../check/testdata/impl/lookup/import.carbon | 20 +++--- .../packages/cross_package_import.carbon | 2 +- .../testdata/packages/explicit_imports.carbon | 4 +- .../packages/fail_import_repeat.carbon | 4 +- 6 files changed, 59 insertions(+), 45 deletions(-) diff --git a/toolchain/check/check.cpp b/toolchain/check/check.cpp index d005e6e9946d..25667b66c58c 100644 --- a/toolchain/check/check.cpp +++ b/toolchain/check/check.cpp @@ -158,9 +158,12 @@ struct UnitInfo { struct PackageImports { // Use the constructor so that the SmallVector is only constructed // as-needed. - explicit PackageImports(Parse::ImportDirectiveId node_id) - : node_id(node_id) {} + explicit PackageImports(IdentifierId package_id, + Parse::ImportDirectiveId node_id) + : package_id(package_id), node_id(node_id) {} + // The identifier of the imported package. + IdentifierId package_id; // The first `import` directive in the file, which declared the package's // identifier (even if the import failed). Used for associating diagnostics // not specific to a single import. @@ -187,11 +190,14 @@ struct UnitInfo { ErrorTrackingDiagnosticConsumer err_tracker; DiagnosticEmitter emitter; - // A map of package names to outgoing imports. If a package includes - // unavailable library imports, it has an entry with has_load_error set. - // Invalid imports (for example, `import Main;`) aren't added because they - // won't add identifiers to name lookup. - llvm::DenseMap package_imports_map; + // List of the outgoing imports. If a package includes unavailable library + // imports, it has an entry with has_load_error set. Invalid imports (for + // example, `import Main;`) aren't added because they won't add identifiers to + // name lookup. + llvm::SmallVector package_imports; + + // A map of the package names to the outgoing imports above. + llvm::DenseMap package_imports_map; // The remaining number of imports which must be checked before this unit can // be processed. @@ -212,22 +218,25 @@ static auto ImportCurrentPackage(Context& context, UnitInfo& unit_info, SemIR::InstId package_inst_id, SemIR::TypeId namespace_type_id) -> void { // Add imports from the current package. - auto self_import = unit_info.package_imports_map.find(IdentifierId::Invalid); - if (self_import == unit_info.package_imports_map.end()) { + auto self_import_map_it = + unit_info.package_imports_map.find(IdentifierId::Invalid); + if (self_import_map_it == unit_info.package_imports_map.end()) { // Push the scope; there are no names to add. context.scope_stack().Push(package_inst_id, SemIR::NameScopeId::Package); return; } + UnitInfo::PackageImports& self_import = + unit_info.package_imports[self_import_map_it->second]; // Track whether an IR was imported in full, including `export import`. This // distinguishes from IRs that are indirectly added without all names being // exported to this IR. llvm::SmallVector imported_irs(total_ir_count, false); - if (self_import->second.has_load_error) { + if (self_import.has_load_error) { context.name_scopes().Get(SemIR::NameScopeId::Package).has_error = true; } - for (const auto& import : self_import->second.imports) { + for (const auto& import : self_import.imports) { const auto& import_sem_ir = **import.unit_info->unit->sem_ir; auto& imported_ir = imported_irs[import_sem_ir.check_ir_id().index]; @@ -288,7 +297,7 @@ static auto InitPackageScopeAndImports(Context& context, UnitInfo& unit_info, // First create the constant values map for all imported IRs. We'll populate // these with mappings for namespaces as we go. size_t num_irs = 0; - for (auto& [_, package_imports] : unit_info.package_imports_map) { + for (auto& package_imports : unit_info.package_imports) { num_irs += package_imports.imports.size(); } if (!unit_info.api_for_impl) { @@ -334,8 +343,8 @@ static auto InitPackageScopeAndImports(Context& context, UnitInfo& unit_info, namespace_type_id); CARBON_CHECK(context.scope_stack().PeekIndex() == ScopeIndex::Package); - for (auto& [package_id, package_imports] : unit_info.package_imports_map) { - if (!package_id.is_valid()) { + for (auto& package_imports : unit_info.package_imports) { + if (!package_imports.package_id.is_valid()) { // Current package is handled above. continue; } @@ -348,9 +357,9 @@ static auto InitPackageScopeAndImports(Context& context, UnitInfo& unit_info, CARBON_CHECK(!import.names.is_export) << "Imports from other packages can't be exported."; } - ImportLibrariesFromOtherPackage(context, namespace_type_id, - package_imports.node_id, package_id, - import_irs, package_imports.has_load_error); + ImportLibrariesFromOtherPackage( + context, namespace_type_id, package_imports.node_id, + package_imports.package_id, import_irs, package_imports.has_load_error); } } @@ -998,14 +1007,20 @@ static auto TrackImport( return; } - // Get the package imports. - auto package_imports_it = unit_info.package_imports_map - .try_emplace(import.package_id, import.node_id) - .first; + // Get the package imports, or create them if this is the first. + auto [package_imports_map_it, is_inserted] = + unit_info.package_imports_map.insert( + {import.package_id, unit_info.package_imports.size()}); + if (is_inserted) { + unit_info.package_imports.push_back( + UnitInfo::PackageImports(import.package_id, import.node_id)); + } + UnitInfo::PackageImports& package_imports = + unit_info.package_imports[package_imports_map_it->second]; if (auto api = api_map.find(import_key); api != api_map.end()) { // Add references between the file and imported api. - package_imports_it->second.imports.push_back({import, api->second}); + package_imports.imports.push_back({import, api->second}); ++unit_info.imports_remaining; api->second->incoming_imports.push_back(&unit_info); @@ -1016,7 +1031,7 @@ static auto TrackImport( } } else { // The imported api is missing. - package_imports_it->second.has_load_error = true; + package_imports.has_load_error = true; CARBON_DIAGNOSTIC(LibraryApiNotFound, Error, "Corresponding API for '{0}' not found.", std::string); CARBON_DIAGNOSTIC(ImportNotFound, Error, "Imported API '{0}' not found.", @@ -1196,8 +1211,7 @@ auto CheckParseTrees(llvm::MutableArrayRef units, bool prelude_import, // TODO: Better identify cycles, maybe try to untangle them. for (auto& unit_info : unit_infos) { if (unit_info.imports_remaining > 0) { - for (auto& [package_id, package_imports] : - unit_info.package_imports_map) { + for (auto& package_imports : unit_info.package_imports) { for (auto* import_it = package_imports.imports.begin(); import_it != package_imports.imports.end();) { if (*import_it->unit_info->unit->sem_ir) { diff --git a/toolchain/check/testdata/class/cross_package_import.carbon b/toolchain/check/testdata/class/cross_package_import.carbon index 3689f8b42a98..ee1f7e1486d7 100644 --- a/toolchain/check/testdata/class/cross_package_import.carbon +++ b/toolchain/check/testdata/class/cross_package_import.carbon @@ -289,16 +289,16 @@ var c: Other.C = {}; // CHECK:STDOUT: // CHECK:STDOUT: file { // CHECK:STDOUT: package: = namespace [template] { -// CHECK:STDOUT: .Other = %Other // CHECK:STDOUT: .Core = %Core +// CHECK:STDOUT: .Other = %Other // CHECK:STDOUT: .c = %c // CHECK:STDOUT: } -// CHECK:STDOUT: %Other: = namespace [template] {} // CHECK:STDOUT: %Core: = namespace [template] {} +// CHECK:STDOUT: %Other: = namespace [template] {} // CHECK:STDOUT: %Other.ref: = name_ref Other, %Other [template = %Other] -// CHECK:STDOUT: %import_ref.1: type = import_ref ir1, inst+2, loaded [template = constants.%C] -// CHECK:STDOUT: %import_ref.2 = import_ref ir1, inst+3, unloaded -// CHECK:STDOUT: %import_ref.3 = import_ref ir2, inst+2, unloaded +// CHECK:STDOUT: %import_ref.1: type = import_ref ir2, inst+2, loaded [template = constants.%C] +// CHECK:STDOUT: %import_ref.2 = import_ref ir2, inst+3, unloaded +// CHECK:STDOUT: %import_ref.3 = import_ref ir3, inst+2, unloaded // CHECK:STDOUT: %C.ref: type = name_ref C, %import_ref.1 [template = constants.%C] // CHECK:STDOUT: %c.var: ref C = var c // CHECK:STDOUT: %c: ref C = bind_name c, %c.var diff --git a/toolchain/check/testdata/impl/lookup/import.carbon b/toolchain/check/testdata/impl/lookup/import.carbon index 88bf801f1bb7..11a2dde98532 100644 --- a/toolchain/check/testdata/impl/lookup/import.carbon +++ b/toolchain/check/testdata/impl/lookup/import.carbon @@ -112,26 +112,26 @@ fn G(c: Impl.C) { // CHECK:STDOUT: // CHECK:STDOUT: file { // CHECK:STDOUT: package: = namespace [template] { -// CHECK:STDOUT: .Impl = %Impl // CHECK:STDOUT: .Core = %Core +// CHECK:STDOUT: .Impl = %Impl // CHECK:STDOUT: .G = %G.decl // CHECK:STDOUT: } -// CHECK:STDOUT: %Impl: = namespace [template] {} // CHECK:STDOUT: %Core: = namespace [template] {} -// CHECK:STDOUT: %import_ref.1 = import_ref ir1, inst+14, unloaded -// CHECK:STDOUT: %import_ref.2: = import_ref ir1, inst+11, loaded [template = constants.%.6] -// CHECK:STDOUT: %import_ref.3 = import_ref ir1, inst+4, unloaded -// CHECK:STDOUT: %import_ref.4 = import_ref ir1, inst+6, unloaded -// CHECK:STDOUT: %import_ref.5: = import_ref ir1, inst+22, loaded [template = constants.%.7] -// CHECK:STDOUT: %import_ref.6: type = import_ref ir1, inst+13, loaded [template = constants.%C] +// CHECK:STDOUT: %Impl: = namespace [template] {} +// CHECK:STDOUT: %import_ref.1 = import_ref ir2, inst+14, unloaded +// CHECK:STDOUT: %import_ref.2: = import_ref ir2, inst+11, loaded [template = constants.%.6] +// CHECK:STDOUT: %import_ref.3 = import_ref ir2, inst+4, unloaded +// CHECK:STDOUT: %import_ref.4 = import_ref ir2, inst+6, unloaded +// CHECK:STDOUT: %import_ref.5: = import_ref ir2, inst+22, loaded [template = constants.%.7] +// CHECK:STDOUT: %import_ref.6: type = import_ref ir2, inst+13, loaded [template = constants.%C] // CHECK:STDOUT: %G.decl: G = fn_decl @G [template = constants.%struct.1] { // CHECK:STDOUT: %Impl.ref: = name_ref Impl, %Impl [template = %Impl] // CHECK:STDOUT: %C.ref: type = name_ref C, %import_ref.6 [template = constants.%C] // CHECK:STDOUT: %c.loc4_6.1: C = param c // CHECK:STDOUT: @G.%c: C = bind_name c, %c.loc4_6.1 // CHECK:STDOUT: } -// CHECK:STDOUT: %import_ref.7: type = import_ref ir1, inst+2, loaded [template = constants.%.2] -// CHECK:STDOUT: %import_ref.8 = import_ref ir1, inst+6, unloaded +// CHECK:STDOUT: %import_ref.7: type = import_ref ir2, inst+2, loaded [template = constants.%.2] +// CHECK:STDOUT: %import_ref.8 = import_ref ir2, inst+6, unloaded // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: interface @HasF { diff --git a/toolchain/check/testdata/packages/cross_package_import.carbon b/toolchain/check/testdata/packages/cross_package_import.carbon index f4b83bfaa801..37f62fa345b2 100644 --- a/toolchain/check/testdata/packages/cross_package_import.carbon +++ b/toolchain/check/testdata/packages/cross_package_import.carbon @@ -408,7 +408,7 @@ fn Other.G() {} // CHECK:STDOUT: %import_ref.1: = import_ref ir1, inst+2, loaded // CHECK:STDOUT: %Other: = namespace %import_ref.1, [template] {} // CHECK:STDOUT: %Core: = namespace [template] {} -// CHECK:STDOUT: %import_ref.2: F = import_ref ir2, inst+2, loaded [template = constants.%struct] +// CHECK:STDOUT: %import_ref.2: F = import_ref ir3, inst+2, loaded [template = constants.%struct] // CHECK:STDOUT: %F.decl: F = fn_decl @F [template = constants.%struct] {} // CHECK:STDOUT: } // CHECK:STDOUT: diff --git a/toolchain/check/testdata/packages/explicit_imports.carbon b/toolchain/check/testdata/packages/explicit_imports.carbon index 845bcff4efb7..75c326e65963 100644 --- a/toolchain/check/testdata/packages/explicit_imports.carbon +++ b/toolchain/check/testdata/packages/explicit_imports.carbon @@ -65,11 +65,11 @@ import library "lib"; // CHECK:STDOUT: // CHECK:STDOUT: file { // CHECK:STDOUT: package: = namespace [template] { -// CHECK:STDOUT: .Api = %Api // CHECK:STDOUT: .Core = %Core +// CHECK:STDOUT: .Api = %Api // CHECK:STDOUT: } -// CHECK:STDOUT: %Api: = namespace [template] {} // CHECK:STDOUT: %Core: = namespace [template] {} +// CHECK:STDOUT: %Api: = namespace [template] {} // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: --- main_lib_api.carbon diff --git a/toolchain/check/testdata/packages/fail_import_repeat.carbon b/toolchain/check/testdata/packages/fail_import_repeat.carbon index a6d209ae7c1d..0d74cfd9997d 100644 --- a/toolchain/check/testdata/packages/fail_import_repeat.carbon +++ b/toolchain/check/testdata/packages/fail_import_repeat.carbon @@ -92,11 +92,11 @@ import library default; // CHECK:STDOUT: // CHECK:STDOUT: file { // CHECK:STDOUT: package: = namespace [template] { -// CHECK:STDOUT: .Api = %Api // CHECK:STDOUT: .Core = %Core +// CHECK:STDOUT: .Api = %Api // CHECK:STDOUT: } -// CHECK:STDOUT: %Api: = namespace [template] {} // CHECK:STDOUT: %Core: = namespace [template] {} +// CHECK:STDOUT: %Api: = namespace [template] {} // CHECK:STDOUT: } // CHECK:STDOUT: // CHECK:STDOUT: --- fail_default_import.carbon