Clarify comment of erase() and remove()

[matthiaskramm]

Summary

Clarify comment of erase().

[mehdi_amini]

The name of these APIs has always been confusing to me. I would have called remove detach instead I think.

Anyway, thanks: LG with a nit.

mlir/include/mlir/IR/Operation.h

70

I wouldn't call this an "unsafe operation" more than any other: there is no "safer alternative" and it does not break invariant of the system.

[rriddle]

mlir/include/mlir/IR/Operation.h

70

I would not advocate the use of dropAllReferences. I would just call out the operation is expected to have no uses.

[bondhugula]

mlir/include/mlir/IR/Operation.h

69–70

it's -> its

70–72

+1 I don't think this change is a net improvement. I see "deallocate its memory" as being obvious and low level. The original comment with the addition that "The operation is expectation to have no uses." makes it clearer.

[bondhugula]

mlir/include/mlir/IR/Operation.h

73

-1 on this change here - "delete it" appears better and consistent. It's understood that references to the result values (which are going to be freed) will dangle.

[mehdi_amini]

I tend to agree with Uday and River FWIW :)

[matthiaskramm]

Updating phrasing according to Uday's suggestion.

[matthiaskramm]

For reference, the reason I don't like "delete" in this context is that Operations are stored in linked lists. But linked list nomenclature uses "delete" to mean something that's different from deallocation.

But that's not the original point of this change, and a discussion for another day. :)

[mehdi_amini]

But linked list nomenclature uses "delete" to mean something that's different from deallocation.

I think the "delete" is really the C++ nomemclature, but even in the abstract: isn't "delete" including the deallocation in the context of linked list in general? It removes the node from the linked list and destroy it as far as I can tell?

We can use "destroy" instead of "delete" here as well maybe...

Anyway LGTM again

[matthiaskramm]

It removes the node from the linked list and destroy it as far as I can tell?

Right. But my point was that here, the Operation isn't deleted from all the linked lists. In particular, it isn't deleted from the use lists of its OpOperands.

[mehdi_amini]

Right. But my point was that here, the Operation isn't deleted from all the linked lists. In particular, it isn't deleted from the use lists of its OpOperands.

Ah I misunderstood what you were referring to, because the linked-list that is the "owner" of an Operation is the Block.
Here the linked-list of def-use chain isn't touched in anyway: it isn't detached nor destroy. The doc is really about the Block list point of view only.

[mehdi_amini]

mlir/include/mlir/IR/Operation.h

70

Maybe make this even more explicit like this

[matthiaskramm]

mlir/include/mlir/IR/Operation.h

70

Yes, but what I really wanted to warn about is that even if use_empty() returns true, the OpOperand use chain can point to this Operation (through "owner"), and calling erase() will leave a dangling reference.

[mehdi_amini]

mlir/include/mlir/IR/Operation.h

70

I don't understand the dangling reference, can you elaborate?

[rriddle]

mlir/include/mlir/IR/Operation.h

70

OpOperands are owned by the Operation, so any use of an OpOperand (same with OpResult, etc.) after an operation is erased is a dangling reference.

[mehdi_amini]

mlir/include/mlir/IR/Operation.h

70

Sure: but when destroying the Operation, and so the OpOperand, what kind of dangling reference can exist?

I mean sure, someone can have a pointer to the OpOperand or the Operation itself in their own data structure, but surely we won't warn about this right?

[matthiaskramm]

Use Mehdi's phrasing.

[matthiaskramm]

mlir/include/mlir/IR/Operation.h

70

Oh, you're right! The OpOperand is destroyed and unlinked from the use chain. I missed the explicit call to ~OpOperand.

Changed this to use your phrasing.

[bondhugula]

mlir/include/mlir/IR/Operation.h

69–71

Sorry for being nitpicky here but there is a really core operation and it would be good to ensure that the additional clarification doesn't create new ambiguity. "under the assumption ..." here is vague. Methods either have pre-conditions or they don't; pre-conditions are typically asserted on -- however, erase() doesn't assert on use_empty(). With this change, it'd be unclear as to what happens when there are still uses. There could be valid use cases where somehow erase() was called but the user knew what to do to keep the IR valid and not have undefined behavior -- i.e., the users are somehow taken care of later.

In fact, if we want to improve this comment on another aspect, it's the nested/recursive aspect. For eg. the doc comment on the implementation has diverged and it says this:

/// Remove this operation (and its descendants) from its Block and delete
/// all of them.

There isn't a reason to use two different versions. We should unify the two.

70

Sure: but when destroying the Operation, and so the OpOperand, what kind of dangling
reference can exist?

Destroying an Operation does not destroy the OpOperands of operations that use the results of the former operation. Such OpOperands will have a dangling reference.

[bondhugula]

With this change, it'd be unclear as to what happens when there are still uses.

Sorry, it does actually assert on !use_empty() under NDEBUG. We could just say: "The operation should have no uses." -- it's a pre-condition.

[matthiaskramm]

Try to adjust wording to incorporate both Uday's and Medhi's suggestions.

Also, adjust the comment in the implementation to be the same as in the
header.

[matthiaskramm]

The operation should have no uses.

I think "should" is ambiguous here, since it can mean both "you should make sure that the operation has no uses" as well as "after calling erase(), the operation should have no uses anymore". Yes, the latter is silly, but that only becomes clear from context, not from the wording. So using "must" now.

mlir/include/mlir/IR/Operation.h

public:
   Optional<RegisteredOperationName> getRegisteredInfo() {
     return getName().getRegisteredInfo();
   }
 
   /// Returns true if this operation has a registered operation description,
   /// otherwise false.
   bool isRegistered() { return getName().isRegistered(); }
 
   /// Remove this operation from its parent block and delete it.
+  /// There must be no uses of any results (i.e., use_empty() must be true.)
   void erase();
 
   /// Remove the operation from its parent block, but don't delete it.
   void remove();
 
   /// Create a deep copy of this operation, remapping any operands that use
   /// values outside of the operation using the map that is provided (leaving
   /// them alone if no entry is present).  Replaces references to cloned
   /// sub-operations to the corresponding operation that is copied, and adds
   /// those mappings to the map.
   Operation *clone(BlockAndValueMapping &mapper);

mlir/lib/IR/Operation.cpp

void llvm::ilist_traits<::mlir::Operation>::transferNodesFromList(
   if (curParent == otherList.getContainingBlock())
     return;
 
   // Update the 'block' member of each operation.
   for (; first != last; ++first)
     first->block = curParent;
 }
 
-/// Remove this operation (and its descendants) from its Block and delete
-/// all of them.
+/// Remove this operation from its parent block and delete it.
+/// There must be no uses of any results (i.e., use_empty() must be true.)
 void Operation::erase() {
   if (auto *parent = getBlock())
     parent->getOperations().erase(this);
   else
     destroy();
 }
 
 /// Remove the operation from its parent block, but don't delete it.