Trace object paths in StructuralEqual #4
Conversation
| bool operator()(const DataType& lhs, const DataType& rhs) const; | ||
|
|
||
| template <typename ENum, typename = typename std::enable_if<std::is_enum<ENum>::value>::type> | ||
| bool operator()(const ENum& lhs, const ENum& rhs) const { |
There was a problem hiding this comment.
Why does enum comparison have different code path?
There was a problem hiding this comment.
Every enum is a unique type, so we need a template function for enums. We can't move the entire implementation of this function to structural_equal.cc because the client code needs to instantiate the template with different parameters.
To reduce the binary bloat generated by templates (and to hide implementation details from the header), I extracted as much of the common code for this template function into a concrete implementation function EnumAttrsEqual() that is placed in structural_equal.cc.
There was a problem hiding this comment.
Thanks for the explanation! I learned a new cool technique today!
include/tvm/node/object_path.h
Outdated
| const ObjectPathNode* ParentNode() const; | ||
|
|
||
| /*! Compares just the last node of the path, without comparing the whole path. */ | ||
| virtual bool LastNodeEqual(const ObjectPathNode& other) const = 0; |
There was a problem hiding this comment.
Will it be better to have a virtual PathEqual method? Doing so will add one or two lines of duplicate code to do the downcast and recursive call, but the function name seems to be more intuitive to understand. (This also applies to the LastNodeString() method below.
Also, is it possible to accept const ObjectPathNode* to keep up with the convention in TVM?
There was a problem hiding this comment.
We actually perform the comparison in a loop, not with a recursive call, so I believe there would be more duplication that one or two lines.
In general, I'm not sure if that's the right trade off to make here. LastNodeEqual() is a protected function and is not expected to be used anywhere outside object_path.h/.cc, so we probably shouldn't be worried too much about its intuitiveness. On the other hand, introducing code duplication would make the code significantly more fragile, so I would say it's not worth it.
I think the root of the problem stems from a shortcut I took here with object decomposition. Ideally, we would have a separate PathItem class that would only store one element of the path, and then the Path object would be responsible for storing a PathItem at the end of the path, as well as a parent pointer. However, under current circumstances (TVM's object model boilerplate overhead + lack of C++17's std::variant) this approach would be cumbersome and inefficient.
Regarding your second suggestion: Done (changed the arg type to pointer).
| bool ObjectAttrsEqual(const ObjectRef& lhs, const ObjectRef& rhs, bool map_free_vars, | ||
| const ObjectPathPair* paths) const; | ||
|
|
||
| static void GetPathsFromAttrAddressesAndStoreMismatch(const void* lhs_address, |
There was a problem hiding this comment.
Can this function (and the CompareAttributeValues function) be moved into cc file (i.e., remove the declaration from header, and just implement as a internal function in the cc file)?
There was a problem hiding this comment.
Unfortunately, it needs access to SEqualReducer::PathTracing which is private. So it needs to be either a member or a friend of SEqualReducer, according to the C++ visibility rules (which are not my favorite).
* Add initial IRBuilder. * Add function output to irbuilder; update based on new AST. * Add call method; clean up bindings * Add test. * Add multifuction test * Move implementation to C++; infer shape and type * update op python hook * More tests and bug fix * Add comments. * Update shape/type inference. * Restructure code; add python type hint. * Cleanup code. * Rebase; address comments. * Add call intrinsic. * nits. * Remove call op. * Migrate scope to C++ using tvm::With. * Address naming. * Add GetBlocks API. * Unify EmitOutput APIs; add more comments. * Remove shape and type deduction code. * Also remove the shape/type attr interface. * Address comments. * Differentiate global and local function. * Reset counter after building func/block. * Rebase. * Remove shape infer builtin. * Return from void function as empty tuple. Co-authored-by: Michalis Papadimitriou <mikepapadim@hotmail.com>
* [IR] Introduce StructInfo * StructInfoFunctor and Analysis Support * [TVMScript] Parse type/shape annotation with StructInfo * remove runtime type assign * Remove type/shape during parsing (#2) * Normalizer prep: simple checks and legacy function renaming. * Struct info deduction in BlockBuilder. * Two TODOs * StructInfo Normalizer Fixes (#3) * StructInfo AST Fix * Fix Extern Func Deduction and shape mutator. * Update VoidStructInfo & globalvar (#4) * Fix passes and proper sinfo propagation. * Refactor EraseToWellDefined to Enable Remapping * [WIP] First stab at symbolic param tracking * Update EraseToWellDefined to support symbolic shape return (#5) * fix R.shape with ndim (#6) * Remove update shape/type * Address review comment, AnnotateTypeShape=>AnnotateStructInfo * Update include/tvm/script/ir_builder/relax/frame.h Co-authored-by: Ruihang Lai <ruihangl@cs.cmu.edu> * Address comments * Update printer to use structinfo (#7) * Update Error mechanism to prep for obj loc based reporting * Symbolic shape aware function call return value derivation. The main flow works as follows: - Match and populate shape_var_map and var_map by visit each pair of param and call arguments. - Call EraseToWellDefined to map the ret parameter to new result. * [ANALYSIS] Refactor well-form to only look at struct info. * Update comments according to reviews. * Update include/tvm/relax/struct_info.h Co-authored-by: Ruihang Lai <ruihangl@cs.cmu.edu> Co-authored-by: Siyuan Feng <Hzfengsy@sjtu.edu.cn> Co-authored-by: Tianqi Chen <tqchen> Co-authored-by: Ruihang Lai <ruihangl@cs.cmu.edu>
No description provided.