Skip to content

[REFACTOR] Use FFI types in runtime inline module-create wrapper signatures#19449

Merged
tlopex merged 2 commits into
apache:mainfrom
tqchen:ffi-compat-module-create-signatures
Apr 27, 2026
Merged

[REFACTOR] Use FFI types in runtime inline module-create wrapper signatures#19449
tlopex merged 2 commits into
apache:mainfrom
tqchen:ffi-compat-module-create-signatures

Conversation

@tqchen

@tqchen tqchen commented Apr 26, 2026

Copy link
Copy Markdown
Member

Summary

This PR cleans up the public inline wrapper API for runtime backend module creators introduced in #19447. The wrappers previously used a mix of std::unordered_map / std::vector / std::string and ffi::* types with conversion glue inside the wrapper body.

Changes

  • ConstLoaderModuleCreate: both parameters change from std::unordered_map<std::string, T> to ffi::Map<ffi::String, T>; the conversion loops in the wrapper body are removed (net −12 lines of glue code). The callsite in codegen_vm.cc is updated to build ffi::Map directly and passes the existing ffi::Map<ffi::String, Tensor> params argument through without a copy.
  • VulkanModuleCreate: source parameter changes from std::string to ffi::String. The SPIRVShader smap remains std::unordered_map because SPIRVShader is a plain C++ struct, not FFI-storable.
  • OpenCLModuleCreate (SPIRV overload): spirv_text changes from const std::string& to ffi::String. Same SPIRVShader constraint applies.
  • metal_module.h: removes stale <unordered_map>, <vector>, <memory>, <string> includes left over from before [REFACTOR] Isolate backend module creation via ffi.Module.create.<kind> registry #19447.

Internal *ModuleCreateImpl functions and the compiler-side SourceModuleCreate family are unchanged.

…atures

This PR cleans up the public inline wrapper API for runtime backend module
creators introduced in apache#19447. The wrappers previously used a mix of
std::unordered_map / std::vector / std::string and ffi::* types, with
conversion glue inside the wrapper body.

Changes:
- ConstLoaderModuleCreate: both parameters change from
  std::unordered_map<std::string, T> to ffi::Map<ffi::String, T>; the
  conversion loops in the wrapper body are removed (12 lines deleted).
- VulkanModuleCreate: source parameter changes from std::string to
  ffi::String; SPIRVShader smap stays std::unordered_map because
  SPIRVShader is not an FFI-storable type.
- OpenCLModuleCreate (SPIRV overload): spirv_text parameter changes from
  const std::string& to ffi::String; same SPIRVShader constraint applies.
- metal_module.h: remove stale std-library includes (unordered_map, vector,
  memory, string) that were no longer needed after apache#19447.
- codegen_vm.cc (LinkModules): callsite updated to build
  ffi::Map<ffi::String, ffi::Array<ffi::String>> directly instead of
  std::unordered_map<std::string, std::vector<std::string>>, and passes
  the existing ffi::Map<ffi::String, Tensor> params directly to
  ConstLoaderModuleCreate without copying.

Internal CreateImpl functions and the compiler-side SourceModuleCreate
family are unchanged.

@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 refactors the backend and runtime modules to use TVM FFI container types (ffi::Map, ffi::Array, ffi::String) instead of standard C++ containers, simplifying the code by removing manual conversion logic. Review feedback warns that removing standard library headers from the const loader module may cause compilation failures in the corresponding source file. Additionally, suggestions were made to explicitly include tvm/ffi/string.h and to pass FFI containers by const reference to avoid unnecessary atomic reference counting overhead.

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

src/runtime/const_loader_module.h (35-37)

high

Removing these standard library includes from the header will cause compilation failures in src/runtime/const_loader_module.cc. That file relies heavily on std::string, std::unordered_map, and std::vector (e.g., in the ConstLoaderModuleObj definition and various serialization methods) but does not include them itself. Please add these includes to src/runtime/const_loader_module.cc before removing them from this header.

src/runtime/const_loader_module.h (32)

medium

This header uses ffi::String in the ConstLoaderModuleCreate signature but does not explicitly include <tvm/ffi/string.h>. While it may be pulled in transitively by other FFI headers, it is best practice to include it explicitly to ensure the header is self-contained.

#include <tvm/ffi/string.h>
#include <tvm/runtime/base.h>

src/runtime/const_loader_module.h (51-52)

medium

FFI container types like ffi::Map and ffi::Array are ObjectRef handles. Passing them by value causes an atomic reference count increment and decrement. For input parameters that are not moved from, it is more efficient and consistent with other parts of the codebase (like LinkModules in codegen_vm.cc) to pass them by const &.

    const ffi::Map<ffi::String, Tensor>& const_var_tensor,
    const ffi::Map<ffi::String, ffi::Array<ffi::String>>& const_vars_by_symbol) {

src/runtime/opencl/opencl_module.h (74-75)

medium

To avoid unnecessary atomic reference count increments, ffi::String and ffi::Map should be passed by const & when used as input parameters.

    const std::unordered_map<std::string, spirv::SPIRVShader>& shaders, const ffi::String& spirv_text,
    const ffi::Map<ffi::String, FunctionInfo>& fmap) {

src/runtime/vulkan/vulkan_module.h (53)

medium

To avoid unnecessary atomic reference count increments, ffi::String should be passed by const & when used as an input parameter.

                                      const ffi::String& source) {

Four review points:

1. (high) Add std::string / std::unordered_map / std::vector includes to
   const_loader_module.cc — they were transitively pulled in by the old
   const_loader_module.h before it dropped them, and the .cc relies on
   them directly.

2. (medium) Add explicit `#include <tvm/ffi/string.h>` to
   const_loader_module.h so the header is self-contained.

3. (medium) Pass `ffi::Map` / `ffi::Array` / `ffi::String` parameters
   by `const &` in the inline wrappers — these are `ObjectRef`
   handles, pass-by-value triggers an atomic refcount inc/dec.
   Changed: const_loader_module.h (both params), opencl_module.h
   (spirv_text + fmap on the SPIRV overload), vulkan_module.h
   (smap, fmap, source).

4. (medium) Same const-ref consistency applied to the SPIRVShader
   smap (still std::unordered_map since SPIRVShader is not
   FFI-storable).

Local verify: cpptest 128 / 128 pass; build clean with HIDE_PRIVATE_SYMBOLS=ON.
@tlopex tlopex merged commit b7c85e3 into apache:main Apr 27, 2026
9 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