Skip to content

Conversation

@amharc
Copy link
Contributor

@amharc amharc commented Nov 27, 2024

DenseMap::lookup returns by value (because it default-creates the returned value if the key isn't present in the map), which means that we do a lot of copying here. Since we assert that something is present in the returned value two lines below this call, it's safe to use .at here instead.

Copying and then destroying dense maps here is responsible for 60% of the time spent in LTO indexing in a large internal build.

@amharc amharc requested a review from teresajohnson November 27, 2024 14:24
@llvmbot llvmbot added LTO Link time optimization (regular/full LTO or ThinLTO) llvm:transforms labels Nov 27, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 27, 2024

@llvm/pr-subscribers-lto

@llvm/pr-subscribers-llvm-transforms

Author: Krzysztof Pszeniczny (amharc)

Changes

DenseMap::lookup returns by value (because it default-creates the returned value if the key isn't present in the map), which means that we do a lot of copying here. Since we assert that something is present in the returned value two lines below this call, it's safe to use .at here instead.

Copying and then destroying dense maps here is responsible for 60% of the time spent in LTO indexing in a large internal build.


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

1 Files Affected:

  • (modified) llvm/lib/Transforms/IPO/FunctionImport.cpp (+1-2)
diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp
index fee27f72f208b0..9cca3cdc761451 100644
--- a/llvm/lib/Transforms/IPO/FunctionImport.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp
@@ -1540,8 +1540,7 @@ void llvm::gatherImportedSummariesForModule(
     auto &SummariesForIndex =
         LookupOrCreate(ModuleToSummariesForIndex, FromModule);
 
-    const auto &DefinedGVSummaries =
-        ModuleToDefinedGVSummaries.lookup(FromModule);
+    const auto &DefinedGVSummaries = ModuleToDefinedGVSummaries.at(FromModule);
     const auto &DS = DefinedGVSummaries.find(GUID);
     assert(DS != DefinedGVSummaries.end() &&
            "Expected a defined summary for imported global value");

`DenseMap::lookup` returns by value (because it default-creates the
returned value if the key isn't present in the map), which means that we
do a lot of copying here. Since we assert that something is present in
the returned value two lines below this call, it's safe to use `.at`
here instead.

Copying and destroying the dense maps here is responsible for 60% of the
time spent in LTO indexing in a large internal build.
@amharc amharc changed the title [LTO] Use .at instead of .lookup to avoid copies. [LTO] Use .at instead of .lookup to avoid copies. (NFC) Nov 27, 2024
@amharc amharc merged commit 991154d into llvm:main Nov 27, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

llvm:transforms LTO Link time optimization (regular/full LTO or ThinLTO)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants