Skip to content

[REFACTOR][NODE] Use fn_repr inside kRepr lambdas, not ffi::ReprPrint#19462

Merged
tqchen merged 1 commit into
apache:mainfrom
tqchen:tvm-migrate-to-tvm-ffi-repr-mechanism
Apr 28, 2026
Merged

[REFACTOR][NODE] Use fn_repr inside kRepr lambdas, not ffi::ReprPrint#19462
tqchen merged 1 commit into
apache:mainfrom
tqchen:tvm-migrate-to-tvm-ffi-repr-mechanism

Conversation

@tqchen

@tqchen tqchen commented Apr 27, 2026

Copy link
Copy Markdown
Member

Inside a kRepr lambda, fn_repr is the canonical dispatch. Threading it through to sub-element dispatch preserves recursion context and matches the repr migration's design intent (where fn_repr is the core abstraction for repr delegation). This applies the recommendation from code review of #19461 to three sites.

  • src/relax/ir/dataflow_pattern.cc — helper threads fn_repr
  • src/s_tir/meta_schedule/arg_info.cc — lambda parameter named
  • src/target/virtual_device.ccfn_repr(target) replaces target->str()

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request replaces the legacy ReprPrinter with ffi::ReprPrint, involving the deprecation of the old header and widespread updates to object representation dispatches. The review feedback highlights critical omissions where custom representations for TraceNode and several Py* nodes were removed without being migrated to the new kRepr attribute registration, potentially causing regressions in debug output. Additionally, a static method call in a documentation example requires class qualification for correctness.

I am having trouble creating individual review comments. Click here to see my feedback.

src/s_tir/schedule/trace.cc (549-553)

high

The ReprPrinter dispatch for TraceNode is removed here, but no corresponding kRepr registration is added to replace it. This will cause Trace objects to lose their custom Python-like representation and fall back to a generic object dump. Since TraceAsPythonRepr is still defined in this file, you should register it as the kRepr attribute for TraceNode within a TVM_FFI_STATIC_INIT_BLOCK.

include/tvm/node/functor.h (186)

medium

The Script method is a static member of TVMScriptPrinter. Since the lambda is defined outside the class scope in this example, it needs to be qualified with the class name to compile correctly.

 *    return TVMScriptPrinter::Script(n->a, cfg) + " + " + TVMScriptPrinter::Script(n->b, cfg);

src/s_tir/meta_schedule/cost_model/cost_model.cc (66-73)

medium

Removing the custom ReprPrinter dispatch for PyCostModelNode without registering a kRepr attribute that calls f_as_string will result in a regression where the Python-provided string representation is lost. The default reflection-based repr will only show the fields of the C++ wrapper (like the f_as_string PackedFunc pointer). This pattern appears in several other Py* nodes in this PR and should be addressed to maintain debugability.

fn_repr is the canonical dispatch callback threaded by the kRepr
machinery. Calling ffi::ReprPrint directly from inside a kRepr lambda
bypasses any recursion context the printer carries (cycle detection,
indentation state, future hooks), and breaks the design contract that
all recursive repr calls flow through the provided callback.

Three sites: PatternReprPrinterHelper::Print in dataflow_pattern.cc
now holds a fn_repr reference and routes through it; the
RELAX_PATTERN_PRINTER_DEF macro passes fn_repr to the helper. The
VirtualDevice kRepr uses fn_repr(AnyView(node->target)) instead of
target->str(). The TensorInfo shape loop names fn_repr (no sub-element
ObjectRef dispatch needed since shape yields raw int64_t).
@tqchen tqchen force-pushed the tvm-migrate-to-tvm-ffi-repr-mechanism branch from ff016a8 to cf303e7 Compare April 27, 2026 21:48
@tqchen tqchen merged commit 11ac2ad into apache:main Apr 28, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants