gemini-code-assist[bot] commented on code in PR #19799:
URL: https://github.com/apache/tvm/pull/19799#discussion_r3421368032
##########
src/backend/opencl/codegen/codegen_opencl.cc:
##########
@@ -689,9 +689,8 @@ ffi::Module BuildOpenCL(IRModule mod, Target target) {
TVM_FFI_ICHECK(base_func->IsInstance<PrimFuncNode>())
<< "CodeGenOpenCL: Can only take PrimFunc";
auto prim_func = Downcast<PrimFunc>(base_func);
- auto calling_conv = prim_func->GetAttr<int64_t>(tvm::attr::kCallingConv);
- TVM_FFI_ICHECK(calling_conv.has_value() &&
- calling_conv.value() ==
static_cast<int64_t>(CallingConv::kDeviceKernelLaunch))
+ auto calling_conv =
prim_func->GetAttr<CallingConv>(tvm::attr::kCallingConv);
+ TVM_FFI_ICHECK(calling_conv == CallingConv::kDeviceKernelLaunch)
<< "CodeGenOpenCL: expect calling_conv equals
CallingConv::kDeviceKernelLaunch";
Review Comment:
While the refactoring is more concise, the error message could be more
specific. It's helpful to distinguish between the attribute not being present
and it having an incorrect value.
```c
TVM_FFI_ICHECK(calling_conv.has_value()) << "CodeGenOpenCL: expected
kCallingConv attribute to be set.";
TVM_FFI_ICHECK(calling_conv.value() == CallingConv::kDeviceKernelLaunch)
<< "CodeGenOpenCL: expect calling_conv equals
CallingConv::kDeviceKernelLaunch, but got "
<< static_cast<int>(calling_conv.value());
```
##########
src/backend/metal/codegen/codegen_metal.cc:
##########
@@ -474,9 +474,8 @@ ffi::Module BuildMetal(IRModule mod, Target target) {
CodeGenMetal cg(target);
cg.Init(output_ssa);
auto f = Downcast<PrimFunc>(kv.second);
- auto calling_conv = f->GetAttr<int64_t>(tvm::attr::kCallingConv);
- TVM_FFI_ICHECK(calling_conv.has_value() &&
- calling_conv.value() ==
static_cast<int64_t>(CallingConv::kDeviceKernelLaunch))
+ auto calling_conv = f->GetAttr<CallingConv>(tvm::attr::kCallingConv);
+ TVM_FFI_ICHECK(calling_conv == CallingConv::kDeviceKernelLaunch)
<< "CodeGenMetal: expect calling_conv equals
CallingConv::kDeviceKernelLaunch";
Review Comment:
While the refactoring is more concise, the error message could be more
specific. It's helpful to distinguish between the attribute not being present
and it having an incorrect value.
```suggestion
TVM_FFI_ICHECK(calling_conv.has_value()) << "CodeGenMetal: expected
kCallingConv attribute to be set.";
TVM_FFI_ICHECK(calling_conv.value() == CallingConv::kDeviceKernelLaunch)
<< "CodeGenMetal: expect calling_conv equals
CallingConv::kDeviceKernelLaunch, but got "
<< static_cast<int>(calling_conv.value());
```
##########
src/backend/vulkan/codegen/spirv_utils.cc:
##########
@@ -124,9 +124,8 @@ std::pair<std::unordered_map<std::string,
runtime::SPIRVShader>, std::string> Lo
for (auto kv : mod->functions) {
TVM_FFI_ICHECK(kv.second->IsInstance<PrimFuncNode>()) << "CodeGenSPIRV:
Can only take PrimFunc";
auto f = Downcast<PrimFunc>(kv.second);
- auto calling_conv = f->GetAttr<int64_t>(tvm::attr::kCallingConv);
- TVM_FFI_ICHECK(calling_conv.has_value() &&
- calling_conv.value() ==
static_cast<int64_t>(CallingConv::kDeviceKernelLaunch))
+ auto calling_conv = f->GetAttr<CallingConv>(tvm::attr::kCallingConv);
+ TVM_FFI_ICHECK(calling_conv == CallingConv::kDeviceKernelLaunch)
<< "CodeGenSPIRV: expect calling_conv equals
CallingConv::kDeviceKernelLaunch";
Review Comment:
While the refactoring is more concise, the error message could be more
specific. It's helpful to distinguish between the attribute not being present
and it having an incorrect value.
```c
TVM_FFI_ICHECK(calling_conv.has_value()) << "CodeGenSPIRV: expected
kCallingConv attribute to be set.";
TVM_FFI_ICHECK(calling_conv.value() == CallingConv::kDeviceKernelLaunch)
<< "CodeGenSPIRV: expect calling_conv equals
CallingConv::kDeviceKernelLaunch, but got "
<< static_cast<int>(calling_conv.value());
```
##########
src/backend/webgpu/codegen/codegen_webgpu.cc:
##########
@@ -760,9 +760,8 @@ ffi::Module BuildWebGPU(IRModule mod, Target target) {
TVM_FFI_ICHECK(kv.second->IsInstance<PrimFuncNode>())
<< "CodeGenWebGPU: Can only take PrimFunc";
auto f = Downcast<PrimFunc>(kv.second);
- auto calling_conv = f->GetAttr<int64_t>(tvm::attr::kCallingConv);
- TVM_FFI_ICHECK(calling_conv.has_value() &&
- calling_conv.value() ==
static_cast<int64_t>(CallingConv::kDeviceKernelLaunch))
+ auto calling_conv = f->GetAttr<CallingConv>(tvm::attr::kCallingConv);
+ TVM_FFI_ICHECK(calling_conv == CallingConv::kDeviceKernelLaunch)
<< "CodeGenWebGPU: expect calling_conv equals
CallingConv::kDeviceKernelLaunch";
Review Comment:
While the refactoring is more concise, the error message could be more
specific. It's helpful to distinguish between the attribute not being present
and it having an incorrect value.
```c
TVM_FFI_ICHECK(calling_conv.has_value()) << "CodeGenWebGPU: expected
kCallingConv attribute to be set.";
TVM_FFI_ICHECK(calling_conv.value() == CallingConv::kDeviceKernelLaunch)
<< "CodeGenWebGPU: expect calling_conv equals
CallingConv::kDeviceKernelLaunch, but got "
<< static_cast<int>(calling_conv.value());
```
--
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]
