[FEAT] Add structural map, maybe_inplace_mutate, and var_remap APIs - #649
[FEAT] Add structural map, maybe_inplace_mutate, and var_remap APIs#649Kathryn-cat wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the StructuralMapper and StructuralMapperObj APIs in include/tvm/ffi/extra/structural_map.h to support structural mapping and in-place mutation of object-backed values. It also adds corresponding custom hooks (kStructuralMap and kStructuralInplaceMutate), registers test leaf objects, and includes comprehensive unit tests. The review feedback suggests two minor optimizations in structural_map.h: avoiding a const_cast when obtaining the field address, and reusing the calculated field address instead of recalculating it for the field setter.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
2d1fe02 to
94ef772
Compare
d407312 to
a72586a
Compare
1fa228c to
994c4f7
Compare
d635d50 to
5768af7
Compare
9858fd5 to
a86c5cf
Compare
ee20d47 to
9952e82
Compare
0c891b6 to
43b5ba1
Compare
10276ce to
6b570b3
Compare
7b68f08 to
41290ae
Compare
| TEST(StructuralMutator, HandlesPODAndPerInstanceVariableRemapAPIs) { | ||
| TestStructuralMutator mutator; | ||
|
|
||
| EXPECT_EQ(mutator->Mutate(int64_t{42}).cast<int64_t>(), 42); |
There was a problem hiding this comment.
these are too basic test, we just need to integrate test StructuralMap with the new instances
| } | ||
|
|
||
| // The first changed value lazily creates a copy when the source map is shared. | ||
| { |
There was a problem hiding this comment.
Need test case coverage of inplace mutate case
| * \param value The borrowed value to transform. | ||
| * \return The transformed value or an Error. | ||
| */ | ||
| Expected<Any> MaybeInplaceMutateImpl(AnyView value) noexcept { |
There was a problem hiding this comment.
One case we need to fix is when value is a freevar, we need a special path for it. Since callback may again update the Var mapped value, in which case we need to override by setting free var map
There was a problem hiding this comment.
agreed, just tested and now preorder and postorder would both fail when the callback directly replaces the same freevar used multiple times. I think we need to have tests to cover them
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. |
There was a problem hiding this comment.
Let us have a few compact cases covering high-level usage of StructurualMap, need to be able to test
- inplace happens correctly for Map/Array when certain values are unique, while none implace happens when certain values are shared copies in map
- the hook works e2e as part of StructurualMap, no need to have shallow UT
|
One thing that worth double click on is var remapping handling when callback also presence. In an ideal case, the logic should be like follows:
To keep things consistent with structural hash, we can turn on the remapping for both This allows node being declared as DAG node also being mapped once on first occurance. |
| static reflection::TypeAttrColumn column(reflection::type_attr::kShallowCopy); | ||
| AnyView attr = column[type_index]; | ||
| if (TVM_FFI_PREDICT_FALSE(attr.type_index() != TypeIndex::kTVMFFIFunction)) { | ||
| return Unexpected( | ||
| Error("TypeError", | ||
| std::string(reflection::type_attr::kShallowCopy) + " must be an ffi.Function", "")); |
There was a problem hiding this comment.
seems that it does not support classes like ffi::string.
import tvm_ffi
identity = (int, lambda v: v)
tvm_ffi.structural_walk(tvm_ffi.Array([123, "12345678"]), (int, lambda v: None))
tvm_ffi.structural_map(tvm_ffi.Array([123, "12345678"]), (int, lambda v: v))it cannot work, not sure if expected
There was a problem hiding this comment.
seems something to address, might post the full error message?
51744da to
7c1ab6c
Compare
a56ddb2 to
0f1e3d0
Compare
Summary
This PR adds structural transformation support to TVM FFI, complementing the existing structural equality, hashing, and walking APIs.
The new
StructuralMutatorrecursively transforms reflected object graphs, whilestructural_mapprovides a convenient callback-based interface for compiler passes.Structural mapping
structural_mapfollows the same typed callback model asstructural_walk. Callbacks are matched by runtime type and return either the unchanged value or a replacement.Mapping defaults to post-order, so callbacks observe values whose children have already been transformed. This makes bottom-up rewrites such as constant folding straightforward:
The PR exposes:
StructuralMutator
StructuralMutatorprovides the low-level transformation engine through two operations:Mutatetransforms a value without intentionally modifying the input.MaybeInplaceMutatepermits implementations to reuse an object when doing so is safe.Custom mutation behavior
Object types can customize mutation using:
__s_mutate__for canonical non-in-place transformation.__s_maybe_inplace_mutate__for an optional type-specific in-place optimization.The maybe-in-place hook owns its safety policy and may reuse the input, delegate to normal mutation, or return another value. A type providing it must also provide
__s_mutate__.Built-in hooks are registered for Array, List, Map, and Dict.
Variable identity remapping
The mutator maintains an identity-substitution environment for FreeVar objects. Once an identity is mapped, later occurrences reuse the same result.
For example:
If the same
Varoccurs in a function parameter and its body, both occurrences resolve to the same mapped result.The mutator also exposes
get_var_remapandset_var_remap, allowing custom DAG-style variable wrappers to use their underlying identity object as the remapping key.