gemini-code-assist[bot] commented on code in PR #19851:
URL: https://github.com/apache/tvm/pull/19851#discussion_r3447539213
##########
src/runtime/vm/vm.cc:
##########
@@ -971,6 +971,50 @@ ffi::Function VirtualMachineImpl::_LookupFunction(const
ffi::String& name) {
return ffi::Function(nullptr);
}
+class DSOLibraryCache {
+ public:
+ ffi::Module Open(const std::string& library_path) {
+ std::lock_guard<std::mutex> lock(mutex_);
+ auto it = cache_.find(library_path);
+ if (it == cache_.end()) {
+ ffi::Module lib = ffi::Module::LoadFromFile(library_path);
+ cache_.emplace(library_path, lib);
+ return lib;
+ }
+ return it->second;
+ }
+
+ std::unordered_map<std::string, ffi::Module> cache_;
+ std::mutex mutex_;
+};
+
+/*!
+ * \brief Load a runtime Module, then create and initialize a RelaxVM
+ * \param path The path to the runtime Module (a DSO file) to be loaded
+ * \param device The device used to initialize the RelaxVM
+ * \return The RelaxVM as a runtime Module
+ */
+TVM_RUNTIME_DLL ffi::Module LoadVMModule(std::string path, Device device) {
Review Comment:
1. **Avoid Unnecessary Copies**: Pass `path` by `const std::string&` instead
of `std::string` by value to avoid copying the string.
2. **Redundant DLL Export**: `TVM_RUNTIME_DLL` is already specified on the
declaration in `include/tvm/runtime/vm/vm.h`. It is redundant and should be
omitted from the definition.
```c
ffi::Module LoadVMModule(const std::string& path, Device device) {
```
##########
src/runtime/vm/vm.cc:
##########
@@ -971,6 +971,50 @@ ffi::Function VirtualMachineImpl::_LookupFunction(const
ffi::String& name) {
return ffi::Function(nullptr);
}
+class DSOLibraryCache {
Review Comment:
The `DSOLibraryCache` class uses `std::mutex` and `std::lock_guard`, but the
`<mutex>` header is not included in `src/runtime/vm/vm.cc`. This can lead to
compilation failures depending on the compiler and transitive includes.
Please add `#include <mutex>` at the top of `src/runtime/vm/vm.cc` (around
line 32).
##########
src/runtime/extra/disco/builtin.cc:
##########
@@ -129,7 +90,12 @@ void SyncWorker() {
TVM_FFI_STATIC_INIT_BLOCK() {
namespace refl = tvm::ffi::reflection;
refl::GlobalDef()
- .def("runtime.disco.load_vm_module", LoadVMModule)
+ .def("runtime.disco.load_vm_module",
+ [](std::string path, ffi::Optional<Device> device) -> ffi::Module {
Review Comment:
Pass `path` by `const std::string&` instead of `std::string` by value to
avoid unnecessary string copies.
```c
[](const std::string& path, ffi::Optional<Device> device) ->
ffi::Module {
```
##########
include/tvm/runtime/vm/vm.h:
##########
@@ -219,6 +219,14 @@ class VirtualMachine : public ffi::ModuleObj {
std::unordered_map<uint32_t, Any> extensions;
};
+/*!
+ * \brief Load a runtime Module, then create and initialize a RelaxVM
+ * \param path The path to the runtime Module (a DSO file) to be loaded
+ * \param device The device used to initialize the RelaxVM
+ * \return The RelaxVM as a runtime Module
+ */
+TVM_RUNTIME_DLL ffi::Module LoadVMModule(std::string path, Device device);
Review Comment:
Pass `path` by `const std::string&` instead of `std::string` by value to
avoid unnecessary string copies.
```c
TVM_RUNTIME_DLL ffi::Module LoadVMModule(const std::string& path, Device
device);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
