Skip to content

[LTO] Use .at instead of .lookup to avoid copies. (NFC) #117888

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 27, 2024

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