Skip to content

[REFACTOR][IR] Cleanup attrs.h: drop NullValue, AttrsNodeReflAdapter, legacy BaseAttrsNode methods#19607

Merged
tqchen merged 8 commits into
apache:mainfrom
tqchen:tvm-attrs-h-cleanup
May 26, 2026
Merged

[REFACTOR][IR] Cleanup attrs.h: drop NullValue, AttrsNodeReflAdapter, legacy BaseAttrsNode methods#19607
tqchen merged 8 commits into
apache:mainfrom
tqchen:tvm-attrs-h-cleanup

Conversation

@tqchen
Copy link
Copy Markdown
Member

@tqchen tqchen commented May 25, 2026

Overview

This PR cleans up include/tvm/ir/attrs.h by removing four deprecated abstractions:

  1. NullValue<T>() sentinel helpers (replaced by ffi::Optional<T>)
  2. AttrsNodeReflAdapter<DerivedType> shim template (Attrs structs now inherit BaseAttrsNode directly)
  3. BaseAttrsNode::InitBySeq / InitByPackedArgs legacy initialization methods
  4. DictAttrsNode::InitByPackedArgs override

It also migrates 9 pass-config classes from Attrs/AttrsNodeReflAdapter to ffi::Object, since they are pass configuration objects, not IR attributes.

Changes

Commit A — Replace NullValue() call sites ([REFACTOR][IR] Replace NullValue<T>() call sites with default construction)

  • 11 source files: replace NullValue<T>() with T(), std::nullopt, or DataType::Void()
  • manipulate.h/manipulate.cc: FlipAttrs::axis changed from Integer to ffi::Optional<int64_t>

Commit B — Drop NullValue, AttrsNodeReflAdapter, legacy BaseAttrsNode methods ([REFACTOR][IR] Drop NullValue declaration, AttrsNodeReflAdapter, BaseAttrsNode legacy methods)

  • include/tvm/ir/attrs.h: removes NullValue<T>, InitBySeq, InitByPackedArgs, AttrsNodeReflAdapter<T>
  • src/ir/attrs.cc: removes DictAttrsNode::InitByPackedArgs definition
  • AttrsWithDefaultValues<T>() broadened to accept any ffi::ObjectRef subtype (needed for Commit D)
  • Removes unused includes: reflection/accessor.h, <functional>, <vector>

Commit C — Subclass BaseAttrsNode directly ([REFACTOR][IR] Subclass BaseAttrsNode directly, drop AttrsNodeReflAdapter)

  • 17 attrs headers in include/tvm/relax/attrs/ + include/tvm/target/virtual_device.h
  • All struct FooAttrs : public AttrsNodeReflAdapter<FooAttrs>struct FooAttrs : public BaseAttrsNode

Commit D — Migrate pass-config classes to ffi::Object ([REFACTOR] Migrate pass-config classes to subclass ffi::Object)

  • 9 pass-config classes in src/s_tir/, src/tirx/, src/relax/backend/contrib/
  • XConfigNode : public ffi::Object (was AttrsNodeReflAdapter<XConfigNode>)
  • XConfig : public ffi::ObjectRef (was Attrs)
  • Python bindings updated: 7 classes changed from _ir.Attrs to _ffi.Object

Design Decisions

AttrFieldInfo / OpNode::arguments kept: Pre-flight check revealed GetArgStructInfo() in op_common.h and op_common.cc actively reads op->arguments (names, counts). These were not dead metadata — deleting them would break Relax op argument validation. They are kept as-is.

Commit E (trim attrs.h includes) reduced in scope: Removing structural_equal.h, structural_hash.h, and <unordered_map> from attrs.h caused 47 downstream files to fail compilation. Rather than adding explicit includes to 47 files, only clearly-unused includes (reflection/accessor.h, <functional>, <vector>) were removed in Commit B.

Testing

  • Build: clean compile with -DUSE_CUDA=OFF -DUSE_LLVM=ON
  • Tests passing:
    • tests/python/ir/ (93 passed)
    • tests/python/relax/test_analysis.py, test_blockbuilder_core.py, test_op_manipulate.py, test_transform.py (209 passed)
    • tests/python/s_tir/transform/test_s_tir_transform_loop_partition.py, test_s_tir_transform_unify_thread_binding.py (30 passed)
    • tests/python/tirx-transform/test_tir_transform_unroll_loop.py, test_tir_transform_simplify.py, test_tir_transform_remove_no_op.py (108 passed, 6 xfailed)
  • Pre-existing failures (unrelated to this PR): test_s_tir_transform_lower_opaque_block, test_s_tir_transform_compact_buffer_region::TestLetBinding::test_compact, test_tir_transform_vectorize::test_vectorize_llvm_pure_intrin_fail

tqchen added 4 commits May 25, 2026 20:12
…ction

Optional<T> is now fully supported throughout the FFI reflection, so
NullValue<T>() sentinels are no longer needed; this commit replaces every
remaining call site with direct default-construction (T() / Range() /
GlobalVar() / DataType::Void()) ahead of removing the NullValue declaration.

Also migrates FlipAttrs::axis from Integer (boxed nullable IntImm) to
ffi::Optional<int64_t> per the user's directive to prefer Optional<T>
over NullValue<Integer>().
…AttrsNode legacy methods

BaseAttrsNode has only the reflection-based init path remaining. This
commit removes all legacy scaffolding that was needed before the
reflection migration:

- Remove NullValue<T> template and DataType specialization (all call
  sites replaced in the previous commit).
- Remove InitBySeq (undeclared, dead) and InitByPackedArgs (pure
  virtual, only invoked on the dead else-branch) from BaseAttrsNode.
- Remove DictAttrsNode::InitByPackedArgs override and its definition.
- Remove AttrsNodeReflAdapter<DerivedType> template — its sole role was
  providing the InitByPackedArgs stub that now signals an error.
- Simplify AttrsWithDefaultValues to always use the FFI reflection path;
  broaden the static_assert to accept any ffi::ObjectRef subtype so
  pass-config classes can use it too.
- Trim attrs.h includes: remove reflection/accessor.h, <functional>,
  <vector> (unused); keep structural_equal.h, structural_hash.h and
  <unordered_map> as downstream files transitively depend on them.

AttrFieldInfo / OpNode::arguments are kept: they are actively read by
GetArgStructInfo in op_common.h for argument count validation.
Copy link
Copy Markdown
Contributor

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

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 refactors the attribute system by removing the AttrsNodeReflAdapter and the NullValue template, replacing them with BaseAttrsNode or ffi::Object inheritance and default constructors. It also updates the flip operator's axis attribute to be an optional int64_t. However, two critical issues were identified in src/relax/op/tensor/manipulate.cc where attrs->axis.value() is called unconditionally on the newly optional axis field, which will crash if the axis is omitted.

Comment thread src/relax/op/tensor/manipulate.cc Outdated
Comment thread src/relax/op/tensor/manipulate.cc Outdated
tqchen added 2 commits May 25, 2026 22:52
Address Gemini high-priority review on apache#19607: after migrating
FlipAttrs::axis to Optional<int64_t>, the existing call sites
unconditionally call attrs->axis.value(), which throws when axis is
nullopt. Implement the NumPy semantics — axis=None flips every axis —
so the optional field has a well-defined meaning instead of being a
landmine.

- InferStructInfoFlip handles missing axis (shape unchanged).
- InferLayoutFlip handles missing axis (layout preserved as-is).
- flip lowering generates a per-axis sequence of topi.flip calls
  when axis is missing (Option A, matches the single-axis TE pattern).
- Python wrapper defaults to axis=None.
- New tests: test_flip_infer_struct_info_axis_none and
  test_flip_axis_none (end-to-end execution against np.flip).
…ruff-format

Address ruff-format CI failure on apache#19607: the two-line string formed
by adjacent string literals exceeds the single-line preferred form
ruff-format wants when it fits.
tqchen added 2 commits May 26, 2026 11:26
In commit ff4060f, the NullValue<Var>() default initializer for
StorageAccessVisitor::AccessEntry::buffer was replaced with bare
`Var buffer;`. Unlike most ObjectRef subclasses, Var has an
all-default-arg constructor (Var(name="v", dtype=Int(32))), so
`Var()` does NOT produce a null Var — it produces a real Var named
"v". This made every default-constructed AccessEntry look like it
has a defined buffer, breaking the precondition of the ForNode
visitor's range-relaxation step (touched.size() ICHECK).

Surfaced as CUDA CI failures in paged-attention KV cache and prefill
tests post-PR-19607.

Restore the null semantics by using the explicit
Var(ObjectPtr<VarNode>(nullptr)) constructor — equivalent to the
deleted NullValue<Var>().
Earlier in this PR the field was migrated from Integer to
Optional<int64_t> on the rule "Integer fields become Optional<int64_t>".
That rule doesn't fit here — relax.flip's axis is a required argument,
not a nullable one, and the brief NumPy axis=None compatibility added
on top complicates the surface without callers asking for it.

Revert the field, signature, and Python wrapper to int64_t. Drop the
axis-None legalize branch and the corresponding tests.
@tqchen tqchen merged commit b1e1566 into apache:main May 26, 2026
5 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