Skip to content

[MLIR] Add move constructor to BytecodeWriterConfig #126130

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 2 commits into from
Feb 7, 2025

Conversation

karimnosseir
Copy link
Contributor

@karimnosseir karimnosseir commented Feb 6, 2025

The config is currently not movable and because there are constructors the default move won't be generated, which prevents it from being moved. Also, it is not copyable because of the unique_ptr. This PR adds move constructor to allow moving it.

@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Feb 6, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 6, 2025

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Karim Nosseir (karimnosseir)

Changes

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

2 Files Affected:

  • (modified) mlir/include/mlir/Bytecode/BytecodeWriter.h (+1)
  • (modified) mlir/lib/Bytecode/Writer/BytecodeWriter.cpp (+3)
diff --git a/mlir/include/mlir/Bytecode/BytecodeWriter.h b/mlir/include/mlir/Bytecode/BytecodeWriter.h
index 0287e004bb99367..c6cff0bc813143e 100644
--- a/mlir/include/mlir/Bytecode/BytecodeWriter.h
+++ b/mlir/include/mlir/Bytecode/BytecodeWriter.h
@@ -82,6 +82,7 @@ class BytecodeWriterConfig {
   /// printers for the fallback resources within the map.
   BytecodeWriterConfig(FallbackAsmResourceMap &map,
                        StringRef producer = "MLIR" LLVM_VERSION_STRING);
+  BytecodeWriterConfig(BytecodeWriterConfig &&);
   ~BytecodeWriterConfig();
 
   /// An internal implementation class that contains the state of the
diff --git a/mlir/lib/Bytecode/Writer/BytecodeWriter.cpp b/mlir/lib/Bytecode/Writer/BytecodeWriter.cpp
index 0e96aa97abeba6f..8af98dd137568dc 100644
--- a/mlir/lib/Bytecode/Writer/BytecodeWriter.cpp
+++ b/mlir/lib/Bytecode/Writer/BytecodeWriter.cpp
@@ -66,6 +66,9 @@ BytecodeWriterConfig::BytecodeWriterConfig(FallbackAsmResourceMap &map,
     : BytecodeWriterConfig(producer) {
   attachFallbackResourcePrinter(map);
 }
+BytecodeWriterConfig(BytecodeWriterConfig &&config)
+    : impl(std::move(config.impl)) {}
+
 BytecodeWriterConfig::~BytecodeWriterConfig() = default;
 
 ArrayRef<std::unique_ptr<AttrTypeBytecodeWriter<Attribute>>>

@karimnosseir karimnosseir requested a review from River707 February 6, 2025 21:01
@River707
Copy link
Contributor

River707 commented Feb 6, 2025

Can you some context to the description?

Copy link
Contributor

@River707 River707 left a comment

Choose a reason for hiding this comment

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

Change LGTM.

@karimnosseir
Copy link
Contributor Author

Can you some context to the description?

Done.
Thanks

@karimnosseir karimnosseir force-pushed the bytecodewriter_config_move branch from a433db0 to 0ceea68 Compare February 6, 2025 21:44
@karimnosseir karimnosseir merged commit 7fa57cd into llvm:main Feb 7, 2025
8 checks passed
@karimnosseir karimnosseir deleted the bytecodewriter_config_move branch February 7, 2025 05:31
Icohedron pushed a commit to Icohedron/llvm-project that referenced this pull request Feb 11, 2025
The config is currently not movable and because there are constructors
the default move won't be generated, which prevents it from being moved.
Also, it is not copyable because of the unique_ptr. This PR adds move
constructor to allow moving it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants