gemini-code-assist[bot] commented on code in PR #408:
URL: https://github.com/apache/tvm-ffi/pull/408#discussion_r2684530259
##########
examples/cubin_launcher/dynamic_cubin/src/lib_dynamic.cc:
##########
@@ -125,7 +125,7 @@ void MulTwo(tvm::ffi::TensorView x, tvm::ffi::TensorView y)
{
// Launch kernel
tvm::ffi::cuda_api::ResultType result = g_mul_two_kernel->Launch(args, grid,
block, stream);
- TVM_FFI_CHECK_CUDA_ERROR(result);
+ TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(result);
Review Comment:
Similar to my other comment, you can combine the kernel launch on line 127
and this error check into one line for better readability and conciseness.
```cpp
TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(g_mul_two_kernel->Launch(args, grid,
block, stream));
```
This would replace lines 127 and 128.
##########
examples/cubin_launcher/dynamic_cubin/src/lib_dynamic.cc:
##########
@@ -85,7 +85,7 @@ void AddOne(tvm::ffi::TensorView x, tvm::ffi::TensorView y) {
// Launch kernel
tvm::ffi::cuda_api::ResultType result = g_add_one_kernel->Launch(args, grid,
block, stream);
- TVM_FFI_CHECK_CUDA_ERROR(result);
+ TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(result);
Review Comment:
For conciseness, you could combine the kernel launch on line 87 and this
error check into a single line. This would also remove the need for the
`result` variable. Other parts of this PR follow this more concise pattern
(e.g., in `docs/guides/cubin_launcher.rst`).
For example:
```cpp
TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(g_add_one_kernel->Launch(args, grid,
block, stream));
```
This would replace lines 87 and 88.
##########
examples/cubin_launcher/embedded_cubin/cpp_embed/src/lib_embedded.cc:
##########
@@ -112,7 +112,7 @@ void MulTwo(tvm::ffi::TensorView x, tvm::ffi::TensorView y)
{
// Launch kernel
tvm::ffi::cuda_api::ResultType result = kernel.Launch(args, grid, block,
stream);
- TVM_FFI_CHECK_CUDA_ERROR(result);
+ TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(result);
Review Comment:
Similar to my other comment, you can combine the kernel launch on line 114
and this error check into a single statement to make the code more concise.
```cpp
TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(kernel.Launch(args, grid, block,
stream));
```
##########
examples/cubin_launcher/embedded_cubin/include_bin2c/src/lib_embedded.cc:
##########
@@ -70,7 +70,7 @@ void AddOne(tvm::ffi::TensorView x, tvm::ffi::TensorView y) {
// Launch kernel
tvm::ffi::cuda_api::ResultType result = kernel.Launch(args, grid, block,
stream);
- TVM_FFI_CHECK_CUDA_ERROR(result);
+ TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(result);
Review Comment:
To improve conciseness, consider combining the kernel launch on line 72 with
this error check. This pattern is used in other files in this PR.
```cpp
TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(kernel.Launch(args, grid, block,
stream));
```
##########
tests/python/test_cubin_launcher.py:
##########
@@ -184,8 +184,8 @@ def test_cubin_launcher_add_one() -> None:
DLDevice device = x.device();
cudaStream_t stream =
static_cast<cudaStream_t>(TVMFFIEnvGetStream(device.device_type,
device.device_id));
- cudaError_t result = g_kernel_mul_two->Launch(args, grid, block, stream);
- TVM_FFI_CHECK_CUDA_ERROR(result);
+ auto result = g_kernel_mul_two->Launch(args, grid, block, stream);
+ TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(result);
Review Comment:
Similar to my other comment, these two lines can be combined into a single
statement for better readability and conciseness.
```suggestion
TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(g_kernel_mul_two->Launch(args,
grid, block, stream));
```
##########
tests/python/test_cubin_launcher.py:
##########
@@ -158,8 +158,8 @@ def test_cubin_launcher_add_one() -> None:
DLDevice device = x.device();
cudaStream_t stream =
static_cast<cudaStream_t>(TVMFFIEnvGetStream(device.device_type,
device.device_id));
- cudaError_t result = g_kernel_add_one->Launch(args, grid, block, stream);
- TVM_FFI_CHECK_CUDA_ERROR(result);
+ auto result = g_kernel_add_one->Launch(args, grid, block, stream);
+ TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(result);
Review Comment:
For conciseness, you can combine these two lines into one, as has been done
in other example files in this PR. This also removes the need for the `result`
variable.
```suggestion
TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(g_kernel_add_one->Launch(args,
grid, block, stream));
```
##########
examples/cubin_launcher/embedded_cubin/include_bin2c/src/lib_embedded.cc:
##########
@@ -109,7 +109,7 @@ void MulTwo(tvm::ffi::TensorView x, tvm::ffi::TensorView y)
{
// Launch kernel
tvm::ffi::cuda_api::ResultType result = kernel.Launch(args, grid, block,
stream);
- TVM_FFI_CHECK_CUDA_ERROR(result);
+ TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(result);
Review Comment:
Similar to my other comment, you can combine the kernel launch on line 111
and this error check into a single statement to make the code more concise.
```cpp
TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(kernel.Launch(args, grid, block,
stream));
```
##########
examples/cubin_launcher/embedded_cubin/cpp_embed/src/lib_embedded.cc:
##########
@@ -73,7 +73,7 @@ void AddOne(tvm::ffi::TensorView x, tvm::ffi::TensorView y) {
// Launch kernel
tvm::ffi::cuda_api::ResultType result = kernel.Launch(args, grid, block,
stream);
- TVM_FFI_CHECK_CUDA_ERROR(result);
+ TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(result);
Review Comment:
To improve conciseness, consider combining the kernel launch on line 75 with
this error check. This pattern is used in other files in this PR, for example
in `examples/cubin_launcher/example_nvrtc_cubin.py`.
```cpp
TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(kernel.Launch(args, grid, block,
stream));
```
##########
examples/cubin_launcher/embedded_cubin/embed_with_tvm_ffi/src/lib_embedded.cc:
##########
@@ -70,7 +70,7 @@ void AddOne(tvm::ffi::TensorView x, tvm::ffi::TensorView y) {
// Launch kernel
tvm::ffi::cuda_api::ResultType result = kernel.Launch(args, grid, block,
stream);
- TVM_FFI_CHECK_CUDA_ERROR(result);
+ TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(result);
Review Comment:
For conciseness, you could combine the kernel launch on line 72 and this
error check into a single line. This would also remove the need for the
`result` variable. Other parts of this PR follow this pattern.
```cpp
TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(kernel.Launch(args, grid, block,
stream));
```
##########
examples/cubin_launcher/embedded_cubin/embed_with_tvm_ffi/src/lib_embedded.cc:
##########
@@ -109,7 +109,7 @@ void MulTwo(tvm::ffi::TensorView x, tvm::ffi::TensorView y)
{
// Launch kernel
tvm::ffi::cuda_api::ResultType result = kernel.Launch(args, grid, block,
stream);
- TVM_FFI_CHECK_CUDA_ERROR(result);
+ TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(result);
Review Comment:
Similar to my other comment, you can combine the kernel launch on line 111
and this error check into one line for better readability and conciseness.
```cpp
TVM_FFI_CHECK_CUBIN_LAUNCHER_CUDA_ERROR(kernel.Launch(args, grid, block,
stream));
```
--
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]
