[REFACTOR][RUNTIME] Macro cleanup — TVM_DLL alignment, [[maybe_unused]], logging.h legacy macros#19457
Merged
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors TVM's runtime macros and error handling by migrating InternalError and tvm::runtime::Error to tvm::ffi::Error, replacing custom attributes with standard or FFI-specific ones, and cleaning up logging definitions. Feedback indicates potential compilation failures in the Android RPC component due to incomplete error migration. Additionally, the refactored DLL macro logic in base.h is noted as fragile, and the switch from _WIN32 to _MSC_VER may negatively impact MinGW users on Windows.
Member
Author
|
Fixed in commit 6608455. The android_rpc JNI wrapper was missed during Phase 3 of the macro cleanup since it's not built in the default Linux environment. I've migrated the error pattern to use The change:
|
tlopex
approved these changes
Apr 27, 2026
…SED with [[maybe_unused]]
This PR restructures the DLL macro region in include/tvm/runtime/base.h to
mirror the tvm-ffi style with a layered Emscripten/MSVC/visibility-default
fallthrough. It also replaces the legacy TVM_ATTRIBUTE_UNUSED macro with
the C++17 [[maybe_unused]] attribute.
Main changes:
- Restructured TVM_DLL and TVM_RUNTIME_DLL definitions into a single
layered block matching tvm-ffi's TVM_FFI_DLL / TVM_FFI_DLL_EXPORT shape
- Added TVM_DLL_EXPORT and TVM_RUNTIME_DLL_EXPORT companion macros
- Switched _WIN32 to _MSC_VER (MinGW/Clang-on-Windows fall through to
visibility("default") which is correct for those compilers)
- Removed standalone top-of-file #ifdef __EMSCRIPTEN__ block (folded in)
- Replaced TVM_ATTRIBUTE_UNUSED with [[maybe_unused]] at 7 use sites and
deleted the macro definition
…HROW_EXCEPTION, TVM_LOG_STACK_TRACE, TVM_USE_LIBBACKTRACE from logging.h This PR removes the legacy macro definitions from logging.h that have been superseded by tvm-ffi equivalents. Main changes: - TVM_NO_INLINE -> TVM_FFI_NO_INLINE (5 sites) - TVM_ALWAYS_INLINE -> TVM_FFI_INLINE (22 sites) - TVM_THROW_EXCEPTION -> noexcept(false) inline (3 sites) - TVM_LOG_STACK_TRACE: dead config knob (no #if consumers), deleted from logging.h and 3 web/emcc files - TVM_USE_LIBBACKTRACE: dead config knob, deleted from logging.h, android_rpc, and ios_rpc
…through ffi::Error
This PR removes the tvm::runtime::Error and tvm::runtime::InternalError
thin shims over ffi::Error from logging.h, and migrates all catch sites
to use tvm::ffi::Error directly.
Main changes:
- Delete class InternalError (was a constructor-alias for ffi::Error("InternalError", ...))
- Delete using ffi::Error, using runtime::Error, using runtime::InternalError re-exports from logging.h
- LogFatal::Entry::Finalize() now constructs ffi::Error("InternalError", ...) directly
- 18 catch-site migrations: tvm::runtime::Error / tvm::Error -> tvm::ffi::Error
- ScheduleError base class migrated from tvm::runtime::Error to tvm::ffi::Error
- hexagon_common.cc direct throw migrated to tvm::ffi::Error construction
…lang compatibility Move TVM_FFI_INLINE and TVM_FFI_NO_INLINE to prefix position (before all storage-class specifiers and return types) in all use sites introduced by Phase 2. The [[vendor::attr]] C++ attribute form requires prefix placement; postfix usage (e.g. `void TVM_FFI_INLINE foo()` or `TVM_DLL TVM_FFI_NO_INLINE static foo()`) is rejected by MSVC and Clang on macOS.
Phase 3 of the macro cleanup deleted the runtime::InternalError shim from logging.h, but the android_rpc JNI wrapper was missed (the file is not in the default Linux build, so was not caught locally). Switch to the new tvm::ffi::Error pattern matching hexagon_common.cc.
The two macro families served different shared libraries (libtvm_compiler vs libtvm_runtime) but were entangled under a single #if !defined(TVM_DLL) guard, making it impossible to override one without affecting the other. Split into independent guard blocks so each family can be pre-defined separately by downstream embedders. Move the single emscripten.h include above both blocks to avoid duplication.
Phase 3 removed the tvm::Error and tvm::runtime::Error aliases from logging.h, routing everything through tvm::ffi::Error. Two cpptest files (ir_functor_test.cc, target_test.cc) were not migrated and still used bare Error / tvm::Error in catch clauses. The arm CI surfaced the failure when building cpptest. Update both to tvm::ffi::Error, matching the pattern already used elsewhere.
tqchen
force-pushed
the
macro-cleanup-maybe-unused-tvm-dll
branch
from
b9c9005 to
bce063c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three-phase macro cleanup addressing TVM's macro hygiene:
Phase 1: Restructured
TVM_DLLandTVM_RUNTIME_DLLto match the cleaner pattern from tvm-ffi, adding layered Emscripten/MSVC/visibility-default fallthrough. ReplacedTVM_ATTRIBUTE_UNUSEDwith C++17[[maybe_unused]](8 sites).Phase 2: Deleted 5 legacy macros from
logging.h(TVM_NO_INLINE,TVM_ALWAYS_INLINE,TVM_THROW_EXCEPTION,TVM_LOG_STACK_TRACE,TVM_USE_LIBBACKTRACE), migrating 30 use sites to tvm-ffi equivalents or direct syntax.Phase 3: Removed
tvm::runtime::Errorandtvm::runtime::InternalErrorthin shims, routing all code throughtvm::ffi::Error. Migrated 18+ call sites and updated include hygiene.Test plan
__attribute__((visibility("default")))fallthrough for both DLL families and all[[maybe_unused]]sites. ~5 min build time.tests/python/all-platform-minimal-test/(75/75 pass),tests/python/tir-base/(273/273 pass).cpptest IRF.*(8/8 pass).__EMSCRIPTEN__branch (all four DLL macros →EMSCRIPTEN_KEEPALIVE).pre-commit run --all-filespasses (validates clang-format on rewritten base.h and migrated headers)._MSC_VERblock preserves byte-identical dllexport/dllimport semantics vs. original.TVM_ATTRIBUTE_UNUSED,TVM_NO_INLINE,TVM_ALWAYS_INLINE,TVM_THROW_EXCEPTION,TVM_LOG_STACK_TRACE,TVM_USE_LIBBACKTRACE,tvm::runtime::Error,tvm::runtime::InternalError.