gemini-code-assist[bot] commented on code in PR #18444:
URL: https://github.com/apache/tvm/pull/18444#discussion_r2525555061


##########
src/runtime/cpu_sample_device_api.cc:
##########
@@ -0,0 +1,162 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "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.
+ */
+
+/*!
+ * \file cpu_sample_device_api.cc
+ * \brief CPU Sample device API implementation for testing
+ */
+#include <dmlc/thread_local.h>
+#include <tvm/ffi/function.h>
+#include <tvm/ffi/reflection/registry.h>
+#include <tvm/runtime/device_api.h>
+#include <tvm/runtime/logging.h>
+
+#include <cstdlib>
+#include <cstring>
+#include <iostream>
+
+#include "workspace_pool.h"
+
+namespace tvm {
+namespace runtime {
+
+class CPUSampleDeviceAPI final : public DeviceAPI {
+ public:
+  void SetDevice(Device dev) final {
+    std::cout << "[CPU_SAMPLE] SetDevice called: device_type=" << 
dev.device_type
+              << ", device_id=" << dev.device_id << std::endl;
+  }
+
+  void GetAttr(Device dev, DeviceAttrKind kind, ffi::Any* rv) final {
+    std::cout << "[CPU_SAMPLE] GetAttr called: device_type=" << dev.device_type
+              << ", device_id=" << dev.device_id << ", kind=" << kind << 
std::endl;
+
+    if (kind == kExist) {
+      *rv = 1;
+      std::cout << "[CPU_SAMPLE] GetAttr kExist: returning 1" << std::endl;
+    }
+
+    switch (kind) {
+      case kExist:
+        break;
+      case kMaxThreadsPerBlock:
+        *rv = 256;
+        std::cout << "[CPU_SAMPLE] GetAttr kMaxThreadsPerBlock: returning 256" 
<< std::endl;
+        break;
+      case kWarpSize:
+        *rv = 1;
+        std::cout << "[CPU_SAMPLE] GetAttr kWarpSize: returning 1" << 
std::endl;
+        break;
+      default:
+        std::cout << "[CPU_SAMPLE] GetAttr: unsupported attribute kind " << 
kind << std::endl;
+        break;
+    }
+  }
+
+  void* AllocDataSpace(Device dev, size_t nbytes, size_t alignment, DLDataType 
type_hint) final {
+    std::cout << "[CPU_SAMPLE] AllocDataSpace called: nbytes=" << nbytes
+              << ", alignment=" << alignment << std::endl;
+
+    void* ptr;
+#if _MSC_VER
+    ptr = _aligned_malloc(nbytes, alignment);
+    if (ptr == nullptr) throw std::bad_alloc();
+#elif defined(__ANDROID__) && __ANDROID_API__ < 17
+    ptr = memalign(alignment, nbytes);
+    if (ptr == nullptr) throw std::bad_alloc();
+#else
+    int ret = posix_memalign(&ptr, alignment, nbytes);
+    if (ret != 0) throw std::bad_alloc();
+#endif
+    std::cout << "[CPU_SAMPLE] AllocDataSpace: allocated " << nbytes << " 
bytes at " << ptr
+              << std::endl;
+    return ptr;
+  }
+
+  void FreeDataSpace(Device dev, void* ptr) final {
+    std::cout << "[CPU_SAMPLE] FreeDataSpace called: ptr=" << ptr << std::endl;
+#if _MSC_VER
+    _aligned_free(ptr);
+#else
+    free(ptr);
+#endif
+    std::cout << "[CPU_SAMPLE] FreeDataSpace: freed memory at " << ptr << 
std::endl;
+  }
+
+  void StreamSync(Device dev, TVMStreamHandle stream) final {
+    std::cout << "[CPU_SAMPLE] StreamSync called: device_type=" << 
dev.device_type
+              << ", device_id=" << dev.device_id << ", stream=" << stream << 
std::endl;
+  }
+
+  void* AllocWorkspace(Device dev, size_t size, DLDataType type_hint) final;
+  void FreeWorkspace(Device dev, void* data) final;
+
+  bool SupportsDevicePointerArithmeticsOnHost() final {
+    std::cout << "[CPU_SAMPLE] SupportsDevicePointerArithmeticsOnHost: 
returning true"
+              << std::endl;
+    return true;
+  }
+
+  static CPUSampleDeviceAPI* Global() {
+    static auto* inst = new CPUSampleDeviceAPI();
+    return inst;
+  }
+
+ protected:
+  void CopyDataFromTo(const void* from, size_t from_offset, void* to, size_t 
to_offset, size_t size,
+                      Device dev_from, Device dev_to, DLDataType type_hint,
+                      TVMStreamHandle stream) final {
+    std::cout << "[CPU_SAMPLE] CopyDataFromTo called: size=" << size
+              << ", from_offset=" << from_offset << ", to_offset=" << 
to_offset << std::endl;
+    memcpy(static_cast<char*>(to) + to_offset, static_cast<const char*>(from) 
+ from_offset, size);
+    std::cout << "[CPU_SAMPLE] CopyDataFromTo: copied " << size << " bytes" << 
std::endl;
+  }
+};
+
+struct CPUSampleWorkspacePool : public WorkspacePool {
+  CPUSampleWorkspacePool() : WorkspacePool(static_cast<DLDeviceType>(20), 
CPUSampleDeviceAPI::Global()) {

Review Comment:
   ![high](https://www.gstatic.com/codereviewagent/high-priority.svg)
   
   The device type is hardcoded as `20`. It's better to use the `kDLCPUSample` 
enum member defined in `include/tvm/runtime/device_api.h` to avoid magic 
numbers and improve maintainability. If the enum value for `kDLCPUSample` ever 
changes, this code will break.
   
   ```c
     CPUSampleWorkspacePool() : 
WorkspacePool(static_cast<DLDeviceType>(kDLCPUSample), 
CPUSampleDeviceAPI::Global()) {
   ```



##########
src/runtime/cpu_sample_device_api.cc:
##########
@@ -0,0 +1,162 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "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.
+ */
+
+/*!
+ * \file cpu_sample_device_api.cc
+ * \brief CPU Sample device API implementation for testing
+ */
+#include <dmlc/thread_local.h>
+#include <tvm/ffi/function.h>
+#include <tvm/ffi/reflection/registry.h>
+#include <tvm/runtime/device_api.h>
+#include <tvm/runtime/logging.h>
+
+#include <cstdlib>
+#include <cstring>
+#include <iostream>
+
+#include "workspace_pool.h"
+
+namespace tvm {
+namespace runtime {
+
+class CPUSampleDeviceAPI final : public DeviceAPI {
+ public:
+  void SetDevice(Device dev) final {
+    std::cout << "[CPU_SAMPLE] SetDevice called: device_type=" << 
dev.device_type
+              << ", device_id=" << dev.device_id << std::endl;
+  }
+
+  void GetAttr(Device dev, DeviceAttrKind kind, ffi::Any* rv) final {
+    std::cout << "[CPU_SAMPLE] GetAttr called: device_type=" << dev.device_type
+              << ", device_id=" << dev.device_id << ", kind=" << kind << 
std::endl;
+
+    if (kind == kExist) {
+      *rv = 1;
+      std::cout << "[CPU_SAMPLE] GetAttr kExist: returning 1" << std::endl;
+    }
+
+    switch (kind) {
+      case kExist:
+        break;
+      case kMaxThreadsPerBlock:
+        *rv = 256;
+        std::cout << "[CPU_SAMPLE] GetAttr kMaxThreadsPerBlock: returning 256" 
<< std::endl;
+        break;
+      case kWarpSize:
+        *rv = 1;
+        std::cout << "[CPU_SAMPLE] GetAttr kWarpSize: returning 1" << 
std::endl;
+        break;
+      default:
+        std::cout << "[CPU_SAMPLE] GetAttr: unsupported attribute kind " << 
kind << std::endl;
+        break;
+    }
+  }

Review Comment:
   ![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg)
   
   The `GetAttr` function can be simplified by handling all cases, including 
`kExist`, within the `switch` statement. This removes the redundant `if (kind 
== kExist)` check and makes the logic more straightforward and easier to read.
   
   ```c
     void GetAttr(Device dev, DeviceAttrKind kind, ffi::Any* rv) final {
       std::cout << "[CPU_SAMPLE] GetAttr called: device_type=" << 
dev.device_type
                 << ", device_id=" << dev.device_id << ", kind=" << kind << 
std::endl;
   
       switch (kind) {
         case kExist:
           *rv = 1;
           std::cout << "[CPU_SAMPLE] GetAttr kExist: returning 1" << std::endl;
           break;
         case kMaxThreadsPerBlock:
           *rv = 256;
           std::cout << "[CPU_SAMPLE] GetAttr kMaxThreadsPerBlock: returning 
256" << std::endl;
           break;
         case kWarpSize:
           *rv = 1;
           std::cout << "[CPU_SAMPLE] GetAttr kWarpSize: returning 1" << 
std::endl;
           break;
         default:
           std::cout << "[CPU_SAMPLE] GetAttr: unsupported attribute kind " << 
kind << std::endl;
           break;
       }
     }
   ```



-- 
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]

Reply via email to