[TVMScript] Add ObjectPath class #11977
Conversation
|
Also CC: @tqchen |
There was a problem hiding this comment.
Thank you @gbonik for your first PR! The code is written in an amazingly perfect quality and thank you for all your effort making it perfect! I left a couple of comments, mainly with TVM codebase style, nullability, and other stuff :-)
include/tvm/node/object_path.h
Outdated
| /*! \brief Name of the attribute being accessed. Must be a static string. */ | ||
| String attr_key; | ||
|
|
||
| explicit AttributeAccessPathNode(ObjectPathNode* parent, String attr_key); |
There was a problem hiding this comment.
A style issue: in TVM we prefer having constructor in ObjectRefs. In our case, it's like:
AttributeAccessPath::AttributeAccessPath(ObjectPathNode* parent, String attr_key) {
ObjectPtr<AttributeAccessPathNode> n = make_object<AttributeAccessPath>();
n->parent = parent;
n->attr_key = std::move(attr_key);
return AttributeAccessPath(n);
}There was a problem hiding this comment.
This constructor is only meant to be called from ObjectPath::Attr() via make_object. I would make it private, but I can't, because it is called from the internals of make_object() (C++ visibility rules are funny :) )
I guess I could remove this constructor and rely on the default one, and then set the fields after creation. But that would make it a bit more cumbersome, since I'd need to duplicate the logic of the base class (ObjectPathNode) constructor.
Also, I don't really see how this goes with "non-nullability" thing. For example, if I had some attribute that is a non-nullable ObjectRef, wouldn't it lack the default constructor?
There was a problem hiding this comment.
Yeah C++ visibility is definitely too weird. I hate it personally but have to trial-and-error for a couple of times with some friend declaration like this lol
Also, I don't really see how this goes with "non-nullability" thing. For example, if I had some attribute that is a non-nullable ObjectRef, wouldn't it lack the default constructor?
This is painpoint of the current "non-nullability" that I talked with @tqchen previously. Currently to work around this issue, we have to use default initialization to make non-nullable objects in a "temporarily-null" state (example)
|
BTW @gbonik you may just click "resolve conversation" if my comment is addressed :-) No need to explicitly respond haha |
Motivation: Same IR node object can be referenced in several different contexts inside a larger IR object. For example, a variable could be referenced in several statements within a block. This makes it impossible to use an object pointer to uniquely identify a "location" within the larger IR object for error reporting purposes. The `ObjectPath` class addresses this problem by serving as a unique "locator". Tracking issue: apache#11912
Motivation: when two IR objects fail a structural equality check, currently there is no easy way to find out which part of the IR caused the mismatch. In this PR, we modify the `StructuralEqual` infrastructure to also optionally return a pair of `ObjectPath` objects that point to the mismatch. (See apache#11977). In the upcoming PRs, we will pass these paths to the TIR printer, so that it could highlight the mismatch location nicely. Tracking issue: apache#11912
Motivation: when two IR objects fail a structural equality check, currently there is no easy way to find out which part of the IR caused the mismatch. In this PR, we modify the `StructuralEqual` infrastructure to also optionally return a pair of `ObjectPath` objects that point to the mismatch. (See #11977). In the upcoming PRs, we will pass these paths to the TIR printer, so that it could highlight the mismatch location nicely. Tracking issue: #11912
Motivation: Same IR node object can be referenced in several different contexts inside a larger IR object. For example, a variable could be referenced in several statements within a block. This makes it impossible to use an object pointer to uniquely identify a "location" within the larger IR object for error reporting purposes. The `ObjectPath` class addresses this problem by serving as a unique "locator". Tracking issue: apache#11912
Motivation: when two IR objects fail a structural equality check, currently there is no easy way to find out which part of the IR caused the mismatch. In this PR, we modify the `StructuralEqual` infrastructure to also optionally return a pair of `ObjectPath` objects that point to the mismatch. (See apache#11977). In the upcoming PRs, we will pass these paths to the TIR printer, so that it could highlight the mismatch location nicely. Tracking issue: apache#11912
Motivation: Same IR node object can be referenced in several different contexts inside a larger IR object. For example, a variable could be referenced in several statements within a block. This makes it impossible to use an object pointer to uniquely identify a "location" within the larger IR object for error reporting purposes. The `ObjectPath` class addresses this problem by serving as a unique "locator". Tracking issue: apache#11912
Motivation:
Same IR node object can be referenced in several different contexts inside a larger IR object. For example, a variable could be referenced in several statements within a block.
This makes it impossible to use an object pointer to uniquely identify a "location" within the larger IR object for error reporting purposes. The
ObjectPathclass addresses this problem by serving as a unique "locator".Tracking issue: #11912
cc @yelite @junrushao1994