[REFACTOR][NODE] Migrate ReprPrinter to tvm-ffi __ffi_repr__ mechanism#19461
Conversation
Replace all ~169 TVM_STATIC_IR_FUNCTOR(ReprPrinter, vtable).set_dispatch<T> dispatch sites with the tvm-ffi reflection-based __ffi_repr__ mechanism. Key changes: - Add include/tvm/node/repr.h: ostream operator<< for ObjectRef/Any/Variant delegating to ffi::ReprPrint; AccessStep/AccessPath ostream operators - Add src/node/repr.cc: Dump() helpers, node.AsRepr FFI, AccessPath kRepr - Strip repr_printer.h to a compatibility shim (forward-declares only) - Pattern A (auto-default): remove dispatch, let ffi::ReprPrint generate dataclass-style TypeKey(field=val, ...) repr for ~140 internal types - Pattern B (custom kRepr): register TypeAttrDef<T>.def(kRepr, lambda) for user-facing types: Span, Layout, VirtualDevice, TargetKind, TensorInfo, s_tir Trace - Pattern C (TVM_SCRIPT_REPR): use existing macro for IR/TIR/Relax nodes that delegate to TVMScriptPrinter - Fix double-registration bugs introduced during migration (trace.cc, expr.cc, tirx/ir/expr.cc) - Add explicit refl::ObjectDef<T>() calls for CostModelNode/PyCostModelNode and PyMutatorNode to preserve Python @register_object type index allocation that was implicitly provided by the removed set_dispatch side effect - Update test_tir_transform_vectorize.py: null Target now repr as "None" instead of "(nullptr)" per the new Python-compatible repr convention
There was a problem hiding this comment.
Code Review
This pull request migrates the TVM node representation system from the legacy ReprPrinter to a new FFI-based mechanism using ffi::ReprPrint. The changes involve removing numerous static functor dispatches across the codebase and replacing them with kRepr attribute definitions within FFI initialization blocks. Key additions include the tvm/node/repr.h header and src/node/repr.cc implementation, while the legacy repr_printer files are deprecated. Feedback focuses on improving the robustness and consistency of the new implementation, specifically addressing a potential null pointer dereference in access path formatting, removing an unused header, and ensuring the consistent use of the fn_repr callback for recursive printing to maintain architectural integrity.
| ffi::String FormatAccessPath(const ffi::reflection::AccessPath& path) { | ||
| std::vector<ffi::reflection::AccessStep> steps; | ||
| const ffi::reflection::AccessPathObj* cur = path.get(); | ||
| while (cur->step.defined()) { |
There was a problem hiding this comment.
Potential null pointer dereference. If cur->parent is not defined (which happens at the root of the access path), cur will become null in the next iteration, and cur->step.defined() will crash. You should check if cur is valid before accessing its members.
| while (cur->step.defined()) { | |
| while (cur && cur->step.defined()) { |
There was a problem hiding this comment.
This concern is now moot. The refactor in e9a5055 deletes FormatAccessPath entirely — there is no more manual linked-list walk and no cur = cur->parent.get() cast. AccessPathObj's kRepr hook is now recursive: it returns "" at the root and otherwise delegates parent repr and step repr through fn_repr. Static analysis has nothing to flag.
| #include <sstream> | ||
| #include <vector> | ||
|
|
||
| #include "../support/str_escape.h" |
There was a problem hiding this comment.
Confirmed and fixed in commit 1aba295. The orphan include has been removed.
| // REPR_LAMBDA bodies compile without modification. | ||
| struct PatternReprPrinterHelper { | ||
| std::ostringstream stream; | ||
| void Print(const ObjectRef& x) { stream << ffi::ReprPrint(ffi::Any(x)); } |
There was a problem hiding this comment.
For recursive printing within the __ffi_repr__ mechanism, it is recommended to use the provided fn_repr function rather than calling ffi::ReprPrint directly. This ensures that any context or state maintained by the printer (such as cycle detection or indentation, if implemented) is correctly propagated through the recursion.
void Print(const ObjectRef& x) { stream << fn_repr(ffi::AnyView(x)).cast<ffi::String>(); }
ffi::Function fn_repr;
PatternReprPrinterHelper(ffi::Function fn_repr) : fn_repr(fn_repr) {}There was a problem hiding this comment.
Good catch — we had previously dismissed this as equivalent, but the user has confirmed fn_repr should be used. Fixed in ff016a8: PatternReprPrinterHelper now stores the fn_repr callback and its Print method routes through it (fn_repr(AnyView(x)).cast()) rather than calling ffi::ReprPrint directly. The RELAX_PATTERN_PRINTER_DEF macro now names fn_repr and passes it to the helper constructor.
| os << "TensorInfo(\"" << ti->dtype << "\", ["; | ||
| bool first = true; | ||
| for (int64_t v : ti->shape) { | ||
| if (!first) os << ", "; | ||
| os << v; | ||
| first = false; | ||
| } | ||
| os << "])"; | ||
| return os.str(); |
There was a problem hiding this comment.
Instead of manually iterating over the shape array, you can leverage the provided fn_repr function. Since ti->shape is an ffi::Array, the repr system already knows how to format it consistently.
| os << "TensorInfo(\"" << ti->dtype << "\", ["; | |
| bool first = true; | |
| for (int64_t v : ti->shape) { | |
| if (!first) os << ", "; | |
| os << v; | |
| first = false; | |
| } | |
| os << "])"; | |
| return os.str(); | |
| refl::TypeAttrDef<TensorInfoNode>().def(refl::type_attr::kRepr, | |
| [](TensorInfo ti, ffi::Function fn_repr) -> ffi::String { | |
| std::ostringstream os; | |
| os << "TensorInfo(\"" << ti->dtype << "\", " | |
| << fn_repr(ffi::AnyView(ti->shape)).cast<ffi::String>() << ")"; | |
| return os.str(); | |
| }); |
There was a problem hiding this comment.
Prior dismissal reversed per user direction — fn_repr should be threaded. Fixed in ff016a8: the lambda now names its fn_repr parameter. The shape elements are raw int64_t from ffi::Shape iteration, so there is no ObjectRef sub-element dispatch to route through fn_repr; the manual loop format [v1, v2, ...] is preserved as intended (fn_repr(ti->shape) would produce Shape(...) format, which the user explicitly rejected).
| TVM_FFI_STATIC_INIT_BLOCK() { | ||
| namespace refl = tvm::ffi::reflection; | ||
| refl::TypeAttrDef<VirtualDeviceNode>().def( | ||
| refl::type_attr::kRepr, [](VirtualDevice vd, ffi::Function) -> ffi::String { |
There was a problem hiding this comment.
The fn_repr argument is ignored here. While node->target->str() works for Target, it's better to use the provided fn_repr for consistency with the new printing architecture.
| refl::type_attr::kRepr, [](VirtualDevice vd, ffi::Function) -> ffi::String { | |
| refl::type_attr::kRepr, [](VirtualDevice vd, ffi::Function fn_repr) -> ffi::String { |
There was a problem hiding this comment.
Prior dismissal reversed per user direction — fn_repr should be used for consistency with the kRepr design. Fixed in ff016a8: the lambda now names fn_repr and uses fn_repr(AnyView(node->target)).castffi::String() instead of node->target->str(). This produces the Target(...) dataclass repr rather than the raw target-string format, which is consistent with how other ObjectRef sub-fields are rendered via fn_repr.
The include was carried over from the legacy repr_printer.cc but StrEscape is not referenced in the new repr.cc.
Manual linked-list traversal in FormatAccessPath duplicated work that ffi::ReprPrint's __ffi_repr__ dispatch already does for free; the manual walk also invited a null-deref concern from static analysis. Recurse through parent and step via the kRepr hook; delete the manual walker. AccessStep's formatting is now inline in its own kRepr lambda, using the fn_repr argument for map-key sub-repr as intended.
## Summary
`include/tvm/node/` is a leftover separation from when "node" was a
distinct concept from "ir". Today everything in `include/tvm/node/` is
just lower-level IR plumbing routinely included from `ir/`. This PR
collapses the two by moving surviving headers into `ir/`, deleting dead
ones, and redirecting the rest to tvm-ffi where the machinery already
lives.
Main changes:
- Migrate
`include/tvm/node/{functor,cast,script_printer,attr_registry_map,repr}.h`
→ `include/tvm/ir/` (functor renamed to `node_functor.h` to preserve
type-name connection)
- Delete `repr_printer.h`, `structural_equal.h`, `structural_hash.h`
(post-#19461 shims and forwarding stubs; redirect 3 cpptest consumers to
`tvm/ffi/extra/structural_equal.h`)
- Remove inline `AccessStep`/`AccessPath` `operator<<` definitions
(existing `__ffi_repr__` registrations already cover these; migrate to
generic ObjectRef streaming via kRepr)
- Move `src/node/{repr,script_printer}.cc` → `src/ir/`
- Delete `src/node/` and empty `include/tvm/node/` directories entirely
(no shims)
- Update 35 includers across IR, relax, script, target, tirx, and tests
… favor of default representation (#19709) Python-side meta-schedule classes (`PyCostModel`, `PyFeatureExtractor`, `PyMeasureCallback`, `PyScheduleRule`, `PyMutator`, `PyPostproc`) carried an `f_as_string` callback whose only purpose was to produce a repr-style string (`s_tir.meta_schedule.<SubclassName>(0x...)`) for `str(...)`. This mechanism stopped working after #19461 migrated `ReprPrinter` to the tvm-ffi `__ffi_repr__` mechanism and intentionally removed the per-type `set_dispatch<Py*Node>` hooks that called back into `f_as_string`, which broke three `*_as_string` tests: - `test_meta_schedule_cost_model.py::test_meta_schedule_cost_model_as_string` - `test_meta_schedule_feature_extractor.py::test_meta_schedule_feature_extractor_as_string` - `test_meta_schedule_measure_callback.py::test_meta_schedule_measure_callback_as_string` Rather than restoring the old behavior, this PR removes the mechanism entirely: the string it produced is just a repr, and tvm-ffi reflection already provides an auto-generated default repr for every object. Keeping a dedicated Python → FFI → C++ callback chain alive only to reproduce that is not worth the complexity.
… favor of default representation (apache#19709) Python-side meta-schedule classes (`PyCostModel`, `PyFeatureExtractor`, `PyMeasureCallback`, `PyScheduleRule`, `PyMutator`, `PyPostproc`) carried an `f_as_string` callback whose only purpose was to produce a repr-style string (`s_tir.meta_schedule.<SubclassName>(0x...)`) for `str(...)`. This mechanism stopped working after apache#19461 migrated `ReprPrinter` to the tvm-ffi `__ffi_repr__` mechanism and intentionally removed the per-type `set_dispatch<Py*Node>` hooks that called back into `f_as_string`, which broke three `*_as_string` tests: - `test_meta_schedule_cost_model.py::test_meta_schedule_cost_model_as_string` - `test_meta_schedule_feature_extractor.py::test_meta_schedule_feature_extractor_as_string` - `test_meta_schedule_measure_callback.py::test_meta_schedule_measure_callback_as_string` Rather than restoring the old behavior, this PR removes the mechanism entirely: the string it produced is just a repr, and tvm-ffi reflection already provides an auto-generated default repr for every object. Keeping a dedicated Python → FFI → C++ callback chain alive only to reproduce that is not worth the complexity. (cherry picked from commit 219f1d8)
… favor of default representation (apache#19709) Python-side meta-schedule classes (`PyCostModel`, `PyFeatureExtractor`, `PyMeasureCallback`, `PyScheduleRule`, `PyMutator`, `PyPostproc`) carried an `f_as_string` callback whose only purpose was to produce a repr-style string (`s_tir.meta_schedule.<SubclassName>(0x...)`) for `str(...)`. This mechanism stopped working after apache#19461 migrated `ReprPrinter` to the tvm-ffi `__ffi_repr__` mechanism and intentionally removed the per-type `set_dispatch<Py*Node>` hooks that called back into `f_as_string`, which broke three `*_as_string` tests: - `test_meta_schedule_cost_model.py::test_meta_schedule_cost_model_as_string` - `test_meta_schedule_feature_extractor.py::test_meta_schedule_feature_extractor_as_string` - `test_meta_schedule_measure_callback.py::test_meta_schedule_measure_callback_as_string` Rather than restoring the old behavior, this PR removes the mechanism entirely: the string it produced is just a repr, and tvm-ffi reflection already provides an auto-generated default repr for every object. Keeping a dedicated Python → FFI → C++ callback chain alive only to reproduce that is not worth the complexity. (cherry picked from commit 219f1d8)
This PR phases out the legacy
ReprPrintermachinery (include/tvm/node/repr_printer.h,src/node/repr_printer.cc,src/node/container_printing.cc) by migrating ~169 dispatch sites to the tvm-ffi__ffi_repr__mechanism.Summary of changes:
TVM_STATIC_IR_FUNCTOR(ReprPrinter, vtable).set_dispatch<T>sites have been migrated: most non-user-facing types now use the dataclass auto-default; user-facing types (script/printer/,Trace, DPL pattern syntax, Span/SequentialSpan/SourceName,data_layoutLayout/BijectiveLayout,target_kind/virtual_device) retain custom hooks via__ffi_repr__.include/tvm/node/repr.hwithoperator<<(ostream, ObjectRef/Any/Variant)delegating toffi::ReprPrint. Addssrc/node/repr.ccwithDump()debug helpers and thenode.AsReprFFI for Python compatibility.ObjectRefnow reprs asNone(Python-compatible) rather than(nullptr); one test was updated for this (test_tir_transform_vectorize.py::test_illegal_extent). The auto-default uses the full type-key (e.g.arith.ModularSet(...)) per design direction.Test results: