release/18.x: [mlir][NFC] Apply rule of five to *Pass classes (#80998) #83971
Conversation
Define all special member functions for mlir::Pass, mlir::OperationPass, mlir::PassWrapper and PassGen types since these classes explicitly specify copy-ctor. This, subsequently, should silence static analysis checkers that report rule-of-3 / rule-of-5 violations. Given the nature of the types, however, mark other special member functions deleted: the semantics of a Pass type object seems to be that it is only ever created by being wrapped in a smart pointer, so the special member functions are never to be used externally (except for the copy-ctor - it is "special" since it is a "delegating" ctor for derived pass types to use during cloning - see https://reviews.llvm.org/D104302 for details). Deleting other member functions means that `Pass x(std::move(y))` - that used to silently work (via copy-ctor) - would fail to compile now. Yet, as the copy ctors through the hierarchy are under 'protected' access, the issue is unlikely to appear in practice. Co-authored-by: Asya Pronina <[email protected]> Co-authored-by: Harald Rotuna <[email protected]> (cherry picked from commit 90e9e96)
|
@joker-eph What do you think about merging this PR to the release branch? |
|
@llvm/pr-subscribers-mlir-core @llvm/pr-subscribers-mlir Author: None (llvmbot) ChangesBackport 90e9e96 Requested by: @andrey-golubev Full diff: https://github.com/llvm/llvm-project/pull/83971.diff 2 Files Affected:
diff --git a/mlir/include/mlir/Pass/Pass.h b/mlir/include/mlir/Pass/Pass.h
index 121b253eb83fea..070e0cad38787c 100644
--- a/mlir/include/mlir/Pass/Pass.h
+++ b/mlir/include/mlir/Pass/Pass.h
@@ -161,6 +161,9 @@ class Pass {
explicit Pass(TypeID passID, std::optional<StringRef> opName = std::nullopt)
: passID(passID), opName(opName) {}
Pass(const Pass &other) : Pass(other.passID, other.opName) {}
+ Pass &operator=(const Pass &) = delete;
+ Pass(Pass &&) = delete;
+ Pass &operator=(Pass &&) = delete;
/// Returns the current pass state.
detail::PassExecutionState &getPassState() {
@@ -349,9 +352,15 @@ class Pass {
/// - A 'std::unique_ptr<Pass> clonePass() const' method.
template <typename OpT = void>
class OperationPass : public Pass {
+public:
+ ~OperationPass() = default;
+
protected:
OperationPass(TypeID passID) : Pass(passID, OpT::getOperationName()) {}
OperationPass(const OperationPass &) = default;
+ OperationPass &operator=(const OperationPass &) = delete;
+ OperationPass(OperationPass &&) = delete;
+ OperationPass &operator=(OperationPass &&) = delete;
/// Support isa/dyn_cast functionality.
static bool classof(const Pass *pass) {
@@ -388,9 +397,15 @@ class OperationPass : public Pass {
/// - A 'std::unique_ptr<Pass> clonePass() const' method.
template <>
class OperationPass<void> : public Pass {
+public:
+ ~OperationPass() = default;
+
protected:
OperationPass(TypeID passID) : Pass(passID) {}
OperationPass(const OperationPass &) = default;
+ OperationPass &operator=(const OperationPass &) = delete;
+ OperationPass(OperationPass &&) = delete;
+ OperationPass &operator=(OperationPass &&) = delete;
/// Indicate if the current pass can be scheduled on the given operation type.
/// By default, generic operation passes can be scheduled on any operation.
@@ -444,10 +459,14 @@ class PassWrapper : public BaseT {
static bool classof(const Pass *pass) {
return pass->getTypeID() == TypeID::get<PassT>();
}
+ ~PassWrapper() = default;
protected:
PassWrapper() : BaseT(TypeID::get<PassT>()) {}
PassWrapper(const PassWrapper &) = default;
+ PassWrapper &operator=(const PassWrapper &) = delete;
+ PassWrapper(PassWrapper &&) = delete;
+ PassWrapper &operator=(PassWrapper &&) = delete;
/// Returns the derived pass name.
StringRef getName() const override { return llvm::getTypeName<PassT>(); }
diff --git a/mlir/tools/mlir-tblgen/PassGen.cpp b/mlir/tools/mlir-tblgen/PassGen.cpp
index 11af6497cecf50..90aa67115a4007 100644
--- a/mlir/tools/mlir-tblgen/PassGen.cpp
+++ b/mlir/tools/mlir-tblgen/PassGen.cpp
@@ -183,6 +183,10 @@ class {0}Base : public {1} {
{0}Base() : {1}(::mlir::TypeID::get<DerivedT>()) {{}
{0}Base(const {0}Base &other) : {1}(other) {{}
+ {0}Base& operator=(const {0}Base &) = delete;
+ {0}Base({0}Base &&) = delete;
+ {0}Base& operator=({0}Base &&) = delete;
+ ~{0}Base() = default;
/// Returns the command-line argument attached to this pass.
static constexpr ::llvm::StringLiteral getArgumentName() {
@@ -380,6 +384,10 @@ class {0}Base : public {1} {
{0}Base() : {1}(::mlir::TypeID::get<DerivedT>()) {{}
{0}Base(const {0}Base &other) : {1}(other) {{}
+ {0}Base& operator=(const {0}Base &) = delete;
+ {0}Base({0}Base &&) = delete;
+ {0}Base& operator=({0}Base &&) = delete;
+ ~{0}Base() = default;
/// Returns the command-line argument attached to this pass.
static constexpr ::llvm::StringLiteral getArgumentName() {
|
|
ping @joker-eph: do you mind (back)porting this change to 18.x? if yes, let's close this, alternatively, please approve / merge, thank you! |
|
I don't think we have tracked MLIR library ABI for previous releases, but I think this will change the ABI. Do we have a policy on this for MLIR? |
|
Do we have any ABI stability guarantees for LLVM C++ APIs? I would think that a lot of back ports can break the C++ ABI of LLVM in general? I would apply the same rules as LLVM, and this patch is not fixing anything critical. |
Yes, we always try to keep the C++ API stable in point releases. |
Fair enough. Let's not backport it then. btw,
I'm curious why do you think so, maybe I'm missing something. |
|
I think it's an ABI change (but I could be wrong), because some of the implicit constructors are being deleted. What happens if a user of the library was using these deleted constructors? |
I see your point. In this specific case, the special member functions were implicitly deleted anyway: the class used to have an explicit user-specified (non-default) copy ctor and that's about it. There is at least 1 member field in Also, the users, I believe, are only within the inheritance chain, because all of this special member functions (except dtor) are under In any case, if there's already a released 18.0 then this change would only get into some other version and, as you mentioned, this is something you prefer to avoid for API/ABI incompatible changes. |
|
Aha, so, apparently, I cannot even close a PR (lack of commit write access) :/ Could someone close this one as we don't plan on merging it? Not urgent anyway. |
Backport 90e9e96
Requested by: @andrey-golubev