Skip to content

[TargetLibraryInfo] Use std::move (NFC) #95671

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

Conversation

kazutakahirata
Copy link
Contributor

The std::move here saves 0.11% of heap allocations during the
compilation of a large preprocessed file, namely X86ISelLowering.cpp,
for the X86 target.

The std::move here saves 0.11% of heap allocations during the
compilation of a large preprocessed file, namely X86ISelLowering.cpp,
for the X86 target.
@llvmbot
Copy link
Member

llvmbot commented Jun 15, 2024

@llvm/pr-subscribers-llvm-analysis

Author: Kazu Hirata (kazutakahirata)

Changes

The std::move here saves 0.11% of heap allocations during the
compilation of a large preprocessed file, namely X86ISelLowering.cpp,
for the X86 target.


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

1 Files Affected:

  • (modified) llvm/include/llvm/Analysis/TargetLibraryInfo.h (+2-1)
diff --git a/llvm/include/llvm/Analysis/TargetLibraryInfo.h b/llvm/include/llvm/Analysis/TargetLibraryInfo.h
index f5da222d11f55..b29046a969448 100644
--- a/llvm/include/llvm/Analysis/TargetLibraryInfo.h
+++ b/llvm/include/llvm/Analysis/TargetLibraryInfo.h
@@ -316,7 +316,8 @@ class TargetLibraryInfo {
   // Provide value semantics.
   TargetLibraryInfo(const TargetLibraryInfo &TLI) = default;
   TargetLibraryInfo(TargetLibraryInfo &&TLI)
-      : Impl(TLI.Impl), OverrideAsUnavailable(TLI.OverrideAsUnavailable) {}
+      : Impl(TLI.Impl),
+        OverrideAsUnavailable(std::move(TLI.OverrideAsUnavailable)) {}
   TargetLibraryInfo &operator=(const TargetLibraryInfo &TLI) = default;
   TargetLibraryInfo &operator=(TargetLibraryInfo &&TLI) {
     Impl = TLI.Impl;

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please make the same change in the move assignment operator as well?

@kazutakahirata
Copy link
Contributor Author

Could you please make the same change in the move assignment operator as well?

Fixed in the latest iteration. Thanks!

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kazutakahirata kazutakahirata merged commit ecea837 into llvm:main Jun 15, 2024
5 of 7 checks passed
@kazutakahirata kazutakahirata deleted the cleanup_memory_alloc_11bp_TargetLibraryInfo branch June 15, 2024 21:02
@nikic
Copy link
Contributor

nikic commented Jun 15, 2024

Looking at this again, you could probably also use = default ... or drop all the explicit ctors/assignment operators and use the implicit ones.

@kazutakahirata
Copy link
Contributor Author

Looking at this again, you could probably also use = default ... or drop all the explicit ctors/assignment operators and use the implicit ones.

Great idea! We have one user-declared constructor a few lines above, so I would have to use = default to ask the compiler for the default move constructor and move assignment operator. I'll post a follow-up PR.

@kazutakahirata
Copy link
Contributor Author

I just posted #95685 as a follow-up for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants