[TIR] Improve well-formed check's handling of match buffer - #16655
Conversation
|
Hi @Lunderberg , I tested my cases with this pr, and they are all working properly now. Thanks for your help! |
- The `T.match_buffer` at the start of a function may contain repeated use of the same data var. For example, a function that must accept two `DLTensor` objects with the same backing allocation. - The `"buffer_bind_scope"` is an older style of match buffer, and may be the point of definition for variables.
848d539 to
8ffcead
Compare
slyubomirsky
left a comment
slyubomirsky
left a comment
There was a problem hiding this comment.
It's good to have a more systematic approach to a feature like points of definition in a MatchBuffer. I commented on some parts of the implementation that I couldn't entirely follow.
| // tir::Var they annotate. | ||
| context = WithDef(iter_var.value(), path->Attr("node")); | ||
| context.push_back(WithDef(iter_var.value(), path->Attr("node"))); | ||
| } else if (op->attr_key == attr::buffer_bind_scope) { |
There was a problem hiding this comment.
Probably worth commenting that this acts as an older form of MatchBuffer, per the PR description.
There was a problem hiding this comment.
Thank you, and added a comment with description.
| Visit(op->value, path->Attr("value")); | ||
|
|
||
| std::optional<DefContext<IterVar>> context = std::nullopt; | ||
| std::vector<std::variant<DefContext<IterVar>, DefContext<Var>>> context; |
There was a problem hiding this comment.
Is this ever used? I don't see any reads from it.
There was a problem hiding this comment.
There aren't any reads from it, as it holds a scoped context manager. On destruction, the DefContext<T> object removes items from TIRVisitorWithPath::in_scope_definitions_, and calls the ExitDef handler of the child class.
Also, thank you for pointing this one out. When switching from std::optional to std::vector, I forgot to add a while(context.size()) context.pop_back(); loop in case child classes rely on ExitDef being called in the reverse order from EnterDef.
| Visit(op->condition, path->Attr("condition")); | ||
| Visit(op->bounds, path->Attr("bounds")); | ||
| auto context = WithDef(op->buffer, path->Attr("buffer")); | ||
| auto context = WithDefIfUndefined(op->buffer->data, path->Attr("buffer")->Attr("data")); |
There was a problem hiding this comment.
I imagine this accounts for the case where a BufferRealize can act as a point of definition?
There was a problem hiding this comment.
That's correct. In cases where the buffer's backing allocation is defined externally, the BufferRealize is an annotation of the bounds where the external buffer is accessed. Otherwise, BufferRealize is an allocation. Prior to this commit, only the external backing allocation was handled.
| } | ||
| Visit(op->body, path->Attr("body")); | ||
|
|
||
| while (context.size()) { |
There was a problem hiding this comment.
Thanks for the change. It's probably worth also putting in a comment that this is to ensure that the defs expire, otherwise it seems like spooky action at a distance. Not 100% sure, as the name "DefContext" does imply that it's an RAII sort of thing.
There was a problem hiding this comment.
Yeah, I tried to follow the existing FooContext naming structure (e.g. arith::ConstraintContext).
slyubomirsky
left a comment
slyubomirsky
left a comment
There was a problem hiding this comment.
Thank you for the clarifications. The changes seem reasonable and they address issues that have arisen elsewhere.
) * [TIR] Improve well-formed check's handling of match buffer - The `T.match_buffer` at the start of a function may contain repeated use of the same data var. For example, a function that must accept two `DLTensor` objects with the same backing allocation. - The `"buffer_bind_scope"` is an older style of match buffer, and may be the point of definition for variables. * Improved comment, added context.pop_back()
3 participants
The
T.match_bufferat the start of a function may contain repeated use of the same data var. For example, a function that must accept twoDLTensorobjects with the same backing allocation.The
"buffer_bind_scope"is an older style of match buffer, and may be the point of definition for variables.A
BufferRealizenode may act either as an annotation of an external buffer (which indices are used), or as a point of definition for a local buffer (allocation extents of that buffer).