Skip to content

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

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.

Sign up for GitHub

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
Mar 5, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions mlir/include/mlir/Pass/Pass.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@andrey-golubev andrey-golubev Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Member

@zero9178 zero9178 Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Pass &operator=(Pass &&) = delete;

/// Returns the current pass state.
detail::PassExecutionState &getPassState() {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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>(); }
Expand Down
8 changes: 8 additions & 0 deletions mlir/tools/mlir-tblgen/PassGen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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() {
Expand Down