[mlir][NFC] Apply rule of five to *Pass classes

[andrey-golubev]

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.

[llvmbot]

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: Andrei Golubev (andrey-golubev)

Changes

Define all special member functions of C++ types (mlir::Pass and mlir::OperationPass) since one of them is already provided. As part of this, enable move semantics for these types as it probably makes sense since copy semantics is present.

---

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

1 Files Affected:

• (modified) mlir/include/mlir/Pass/Pass.h (+13)

diff --git a/mlir/include/mlir/Pass/Pass.h b/mlir/include/mlir/Pass/Pass.h
index 121b253eb83fea..efe482fd726f7e 100644
--- a/mlir/include/mlir/Pass/Pass.h
+++ b/mlir/include/mlir/Pass/Pass.h
@@ -161,6 +161,13 @@ 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 &other) {
+    this->passID = other.passID;
+    this->opName = other.opName;
+    return *this;
+  }
+  Pass(Pass&&) = default;
+  Pass& operator=(Pass&&) = default;
 
   /// Returns the current pass state.
   detail::PassExecutionState &getPassState() {
@@ -352,6 +359,9 @@ class OperationPass : public Pass {
 protected:
   OperationPass(TypeID passID) : Pass(passID, OpT::getOperationName()) {}
   OperationPass(const OperationPass &) = default;
+  OperationPass& operator=(const OperationPass &) = default;
+  OperationPass(OperationPass&&) = default;
+  OperationPass& operator=(OperationPass&&) = default;
 
   /// Support isa/dyn_cast functionality.
   static bool classof(const Pass *pass) {
@@ -391,6 +401,9 @@ class OperationPass<void> : public Pass {
 protected:
   OperationPass(TypeID passID) : Pass(passID) {}
   OperationPass(const OperationPass &) = default;
+  OperationPass& operator=(const OperationPass &) = default;
+  OperationPass(OperationPass&&) = default;
+  OperationPass& operator=(OperationPass&&) = default;
 
   /// Indicate if the current pass can be scheduled on the given operation type.
   /// By default, generic operation passes can be scheduled on any operation.

[andrey-golubev]

@joker-eph @River707 @jurahul please take a look. we have some static analysis warnings due to this in our downstream so figured we can fix it.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[andrey-golubev]

apparently, PassOptions (a member field) is non-copyable and non-movable (which is probably why there's a user-provided copy semantics to begin with), thus, I have explicitly marked move semantics deleted in a fixup commit.

[zero9178]

This creates an odd inconsistency where a pass can now be copy constructed but not move constructed.
Since copying is a valid implementation of moving, should the move constructors just dispatch to the copy constructor? Just writing:

Pass(Pass&& rhs) : Pass(rhs)
{}

would achieve this. Similar for the assignment operator.

[andrey-golubev]

agree in general. but I can somewhat justify a partial copy (note that we copy just 2 fields instead of 4 or 5) but cannot really justify a partial move? it would be just weird since - my interpretation - as per C++ "moved-from object is in valid but unspecified state" so, technically, partial copy still allows us to use fields from the "origin" while move would prohibit that. thus, we'd just kind of lose half of the attributes? (PassOptions definitely seems like something important)

[zero9178]

By calling the copy constructor from the move constructor the "moved-from object is in valid but unspecified state" post-condition is upheld since not moving the object but just copying its fields (as the copy constructor does), leaves them in a valid state (the same state as before). Note that this is also only just a convention as to how move constructors should be implemented (if implemented) and not part of the semantics of the core language (just some STL types).

Note that what I suggested would do a partial-copy when using std::move, which should be fine as you said, and does not do a partial-move of the fields (which would IMO also be fine, given that the user explicitly asked for it, but that's not relevant here).

[andrey-golubev]

post-condition is upheld since not moving the object but just copying its fields (as the copy constructor does), leaves them in a valid state (the same state as before).

yes, but I'm not sure whether the compiler would (should) see it: I guess by design the moved-from object can only be assigned-to or deleted. so even though we see that move == copy, it doesn't mean that the compiler has to adhere to this? (all it knows about is "there's a move construction / move assignment operation happening") - maybe inlined code is special though.
(also, i'm pretty sure some static analyzer would come around and say what we do is bad, lol.)

Note that what I suggested would do a partial-copy when using std::move

I guess this would still work through the copy-ctor? Pass pseudoMove(std::move(other)); // calls copy-ctor when move-ctor is deleted?

Anyhow, let me ponder on this for a bit, perhaps there's a nice way forward.

[zero9178]

yes, but I'm not sure whether the compiler would (should) see it: I guess by design the moved-from object can only be assigned-to or deleted. so even though we see that move == copy, it doesn't mean that the compiler has to adhere to this? (all it knows about is "there's a move construction / move assignment operation happening") - maybe inlined code is special though.

C++ compilers do not assign any specific semantics to move constructors. The "valid but undefined state" post-condition is implemented by user-code, not by the compiler. From the POV of the compiler move constructors and assignment operators don't inherently modify the argument. They are just different overloads selected based on the type of the argument. std::move casts an lvalue-ref to an rvalue-ref to make this overload selection choose the move constructor. What the move constructor does is 100% up-to-user code (or the auto-generated one).

(also, i'm pretty sure some static analyzer would come around and say what we do is bad, lol.)

Given it still adheres to the common post-condition of "valid but undefined state" that static-analyzers implement it should be fine. It's probably also not productive to assume the worst-case without evidence.

I guess this would still work through the copy-ctor? Pass pseudoMove(std::move(other)); // calls copy-ctor when move-ctor is deleted?

That is the goal. See e.g. https://godbolt.org/z/6MKMjEYhj In the current state the move constructor is deleted. A deleted function still participates in overload-resolution but causes an error when selected. Explicitly delegating the move constructor to the copy constructor solves that.

[andrey-golubev]

I think you're right.

See e.g. https://godbolt.org/z/6MKMjEYhj In the current state the move constructor is deleted

huh, interesting. actually, commenting out the move-ctor makes the code compile. so the implicitly deleted move-ctor works while explicitly deleted doesn't.

btw. I looked at the PassOptions and I see no reason why it's currently non-movable. it seems to store just a vector of pointers. the deleted copy construction seems due to the custom "clone" mechanics and that it is expensive.
i guess i'm going to make move semantics available for pass options, then we don't have to delegate here but do a proper move. (albeit i could've missed something else)

[andrey-golubev]

on further investigation it turns out there's a bunch of not-really-movable members there. also, note that the copy-ctor is protected. additionally, there's a clone() method that returns a unique_ptr. so, I guess, the expected semantics of the class is to always wrap it into a smart pointer.

thus, i propose we mark all of the special member functions deleted except for the copy-ctor which seems to be used by the clone functionality and call it a day.

[joker-eph]

Is this just making explicit what is the already current API of the class?

[andrey-golubev]

Is this just making explicit what is the already current API of the class?

Yes, pretty much. This should make some of our static analysis tooling happy (meh) and make the special member functions explicitly stated (which is kind of the right thing to do in general so this was enough motivation for me to actually consider changing something here in the upstream).

[joker-eph]

The description seems obsolete now?

[andrey-golubev]

The description seems obsolete now?

Yep, to a degree. I want to solve the move semantics first, then squash changes and update the commit msg (the msg would likely depend on the outcome of the discussion).

[andrey-golubev]

Updated this patch (by force-pushing) to = delete all special member functions except for copy-ctor. The copy-ctor is indeed special as it participates in cloning (e.g. see this patch)

[joker-eph]

Please update the title to add (NFC)

Also update the description to reflect the last revision and be explicit on the fact that this is just making explicit the current state.

[andrey-golubev]

Please update the title to add (NFC)

Also update the description to reflect the last revision and be explicit on the fact that this is just making explicit the current state.

Took me a while to get back to this! Updated as request. Although, technically, @zero9178 is right that explicitly deleting member functions could actually break compilation - which makes me question the "NFC" label. Yet, given the nature of these classes, it seems like the intended behavior is exactly to create them via smart pointers + having a copy-ctor as a very special way to allow custom behavior during cloning (e.g. look at PassWrapper::clonePass() which is a virtual function override and then there are similar facilities in other places).

[joker-eph]

Although, technically, @zero9178 is right that explicitly deleting member functions could actually break compilation

I thought these were implicitly deleted right now?

• which makes me question the "NFC" label

NFC is about the behavior of the compiler, changing APIs is just "business as usual" (any NFC refactoring we do is breaking C++ APIs).

[andrey-golubev]

Although, technically, @zero9178 is right that explicitly deleting member functions could actually break compilation

I thought these were implicitly deleted right now?

Well, move construction semantics would dispatch to copy-ctor afaict. Other than that,

• Defining one of {destructor, copy-ctor, copy-assign-operator} prevents implicit move semantics (so e.g. Pass has implicitly deleted move ctor / assignment operator)
• PassOptions, being non-copyable, prevents copy assignment operator from being generated (PassOptions is also non-movable so kind of reinforces the previous bullet I guess)

(this is the state without modifications)

So, precisely speaking, Pass x(std::move(y)) would work now (via user-defined copy-ctor) and wouldn't work with this patch. But: given that copy-ctors in these classes are under protected access, only the inheritance chains (+ friends?) benefit from this, so I wouldn't bother.
Bottomline: judging by the CI, nothing is using the Pass x(std::move(y)) within MLIR/LLVM and changing this to Pass x(y) seems trivial enough for me (if any user code relies on this).

• which makes me question the "NFC" label

NFC is about the behavior of the compiler, changing APIs is just "business as usual" (any NFC refactoring we do is breaking C++ APIs).

Aha, I see. Well, technically there are several compilers but I get the point :)

[andrey-golubev]

I see a Windows build failure in mlir-cpu-runner/expand-arith-ops.mlir in CI but I don't think this change is causing it (i'd expect compilation errors if anything).

@joker-eph if you're satisfied with answers (and ok with Windows job test failure), could you please merge the PR? (I have no commit access)

[joker-eph]

Is the description up-to-date now?

[joker-eph]

Some more context / motivation of the change in description would be welcome I think: this defines what it is doing, but does not explain why or what is the before/after really.

[andrey-golubev]

Some more context / motivation of the change in description would be welcome I think: this defines what it is doing, but does not explain why or what is the before/after really.

Updated the commit msg and PR description. Please tell if it's better (or not!). Also rebased to check for issues against updated main (just in case).

[andrey-golubev]

Could you please merge it as well? :) (lack of write access...)