Clarify comment of erase().
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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. |
mlir/include/mlir/IR/Operation.h | ||
---|---|---|
74 | -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. |
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. :)
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
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.
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.
mlir/include/mlir/IR/Operation.h | ||
---|---|---|
70 | Maybe make this even more explicit like this |
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. |
mlir/include/mlir/IR/Operation.h | ||
---|---|---|
70 | I don't understand the dangling reference, can you elaborate? |
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. |
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? |
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. |
mlir/include/mlir/IR/Operation.h | ||
---|---|---|
69–70 | 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 |
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. |
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.
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.
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.
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.