Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive V2 printer for TVM Script, leveraging a new traits-based reflection and printing system. It includes extensive updates to IR node reflection registrations and adds custom __ffi_text_print__ overrides to ensure correct formatting for various IR constructs. While the implementation is functional, I have raised concerns regarding the use of fragile manual scope management in DataflowBlock printing and the reliance on thread_local global state for function annotation printing, both of which pose maintainability risks.
| // Pop frame BUT re-register output vars in the PARENT frame so they're | ||
| // accessible to the enclosing SeqExpr's return statement. | ||
| ffi::List<ffi::Any> output_vars; | ||
| for (const auto& b : node->bindings) { | ||
| if (!b->var->IsInstance<DataflowVarNode>()) { | ||
| output_vars.push_back(b->var); | ||
| } | ||
| } | ||
| // Save the var info before popping | ||
| ffi::Map<ffi::Any, text::VarInfo> saved_info; | ||
| for (const auto& ov : output_vars) { | ||
| auto it = printer->obj2info.find(ov); | ||
| if (it != printer->obj2info.end()) { | ||
| saved_info.Set(ov, (*it).second); | ||
| } | ||
| } | ||
| printer->FramePop(); | ||
| // Re-register output vars in parent frame | ||
| if (!printer->frames.empty()) { | ||
| ffi::ObjectRef parent_frame = printer->frames.back().cast<ffi::ObjectRef>(); | ||
| for (const auto& kv : saved_info) { | ||
| printer->obj2info.Set(kv.first, kv.second); | ||
| // Add to parent frame's var list | ||
| auto frame_vars = printer->frame_vars[parent_frame].cast<ffi::List<ffi::Any>>(); | ||
| frame_vars.push_back(kv.first); | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code manually manages variable scopes by saving variable info, popping a frame, and then re-registering variables in the parent frame. This is very fragile and tightly coupled to the internal implementation of the IRPrinter.
While the comment explains why this is necessary (to make dataflow output vars available to the subsequent return statement), this workaround is a significant maintainability risk. A future change in the printer's frame management could easily break this. This might indicate a need for a more robust scope management mechanism in the core printer framework to handle such cases gracefully.
|
|
||
| // Thread-local flag: true when printing function param/return annotations. | ||
| // Used by PrintShapeValue to decide whether to stringify symbolic TIR vars. | ||
| inline thread_local bool g_printing_func_annotation = false; |
There was a problem hiding this comment.
The use of a thread_local global variable g_printing_func_annotation introduces global state, which can make the code harder to reason about, test, and maintain. It creates an implicit dependency for PrintShapeValue on its callers.
While this might be a pragmatic solution given the printer's architecture, consider if it's possible to pass a configuration/context object down the call stack instead. If not, it would be beneficial to add a comment explaining why this approach is necessary and the potential risks involved.
junrushao
force-pushed
the
junrus/2025-04-02/always-use-bundled-tvm-ffi
branch
7 times, most recently
from
e014de7 to
61ce823
Compare
Switch to always using the bundled tvm-ffi submodule for builds. Update internal include paths and Python imports to match the tvm-ffi submodule restructure: headers moved from `tvm/ffi/ir/text/` to `tvm/ffi/extra/` and Python modules from `tvm_ffi.ir.text` to `tvm_ffi.pyast`. No public API changes. All 99 test suites (19,800 tests) pass.
junrushao
force-pushed
the
junrus/2025-04-02/always-use-bundled-tvm-ffi
branch
from
61ce823 to
21958f1
Compare
1 participant
No description provided.