gemini-code-assist[bot] commented on code in PR #191: URL: https://github.com/apache/tvm-ffi/pull/191#discussion_r2463223170
########## examples/stable_c_abi/src/add_one_cpu.c: ########## @@ -0,0 +1,56 @@ +/* + * 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. + */ +// NOLINTBEGIN(bugprone-reserved-identifier,google-readability-braces-around-statements) + +#include <tvm/ffi/c_api.h> +#include <tvm/ffi/extra/c_env_api.h> + +// clang-format off +// [example.begin] +// File: src/add_one_cpu.cc +TVM_FFI_DLL int __tvm_ffi_add_one_cpu(void* handle, const TVMFFIAny* args, int32_t num_args, + TVMFFIAny* result) { + // Step 1. Extract inputs from `Any` + // Step 1.1. Extract `x := args[0]` + DLTensor* x; + if (args[0].type_index == kTVMFFIDLTensorPtr) x = (DLTensor*)(args[0].v_ptr); + else if (args[0].type_index == kTVMFFITensor) x = (DLTensor*)(args[0].v_c_str + sizeof(TVMFFIObject)); + else { TVMFFIErrorSetRaisedFromCStr("ValueError", "Expects a Tensor input"); return -1; } Review Comment:  There's an issue here with how the `DLTensor*` is being extracted from `TVMFFIAny`. You are using `args[0].v_c_str` to access the object pointer when `type_index` is `kTVMFFITensor`. However, `v_c_str` is intended for C-style strings. The correct field for object pointers is `v_obj`. Using `v_c_str` might work if it happens to share the same memory address as `v_obj` in the union, but it relies on implementation-defined behavior and is confusing. Please use `v_obj` for correctness and clarity. Also, casting the pointer to `char*` is necessary for correct pointer arithmetic. ```c if (args[0].type_index == kTVMFFIDLTensorPtr) x = (DLTensor*)(args[0].v_ptr); else if (args[0].type_index == kTVMFFITensor) x = (DLTensor*)((char*)args[0].v_obj + sizeof(TVMFFIObject)); else { TVMFFIErrorSetRaisedFromCStr("ValueError", "Expects a Tensor input"); return -1; } ``` ########## examples/stable_c_abi/src/add_one_cpu.c: ########## @@ -0,0 +1,56 @@ +/* + * 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. + */ +// NOLINTBEGIN(bugprone-reserved-identifier,google-readability-braces-around-statements) + +#include <tvm/ffi/c_api.h> +#include <tvm/ffi/extra/c_env_api.h> + +// clang-format off +// [example.begin] +// File: src/add_one_cpu.cc +TVM_FFI_DLL int __tvm_ffi_add_one_cpu(void* handle, const TVMFFIAny* args, int32_t num_args, + TVMFFIAny* result) { + // Step 1. Extract inputs from `Any` + // Step 1.1. Extract `x := args[0]` + DLTensor* x; + if (args[0].type_index == kTVMFFIDLTensorPtr) x = (DLTensor*)(args[0].v_ptr); + else if (args[0].type_index == kTVMFFITensor) x = (DLTensor*)(args[0].v_c_str + sizeof(TVMFFIObject)); + else { TVMFFIErrorSetRaisedFromCStr("ValueError", "Expects a Tensor input"); return -1; } + // Step 1.2. Extract `y := args[1]` + DLTensor* y; + if (args[1].type_index == kTVMFFIDLTensorPtr) y = (DLTensor*)(args[1].v_ptr); + else if (args[1].type_index == kTVMFFITensor) y = (DLTensor*)(args[1].v_c_str + sizeof(TVMFFIObject)); + else { TVMFFIErrorSetRaisedFromCStr("ValueError", "Expects a Tensor output"); return -1; } Review Comment:  Similar to the extraction of `x`, you are using `v_c_str` to access the object pointer for `y`. This should be `v_obj` to correctly and safely access the `TVMFFIObject*`. Also, casting the pointer to `char*` is necessary for correct pointer arithmetic. ```c if (args[1].type_index == kTVMFFIDLTensorPtr) y = (DLTensor*)(args[1].v_ptr); else if (args[1].type_index == kTVMFFITensor) y = (DLTensor*)((char*)args[1].v_obj + sizeof(TVMFFIObject)); else { TVMFFIErrorSetRaisedFromCStr("ValueError", "Expects a Tensor output"); return -1; } ``` ########## docs/get_started/stable_c_abi.rst: ########## @@ -0,0 +1,190 @@ +.. 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. + +Stable C ABI +============ + +Apache TVM FFI is centered around a single key idea: + + +.. admonition:: Key idea + :class: important + + Every function call can be represented by a single ABI stable C function: + + .. code-block:: C + + int universal_c_abi( // returns 0 if succeed, error code if failure + void* handle, // library handle + Any* args, // inputs: args[0 ... N - 1] + int N, // number of inputs + Any* result, // output: *result + ); + + where ``Any`` is a tagged union of all supported types, e.g. integers, floats, Tensors, strings, etc. + +Built on top of this stable C ABI idea, TVM FFI provides an extensible, performant and ecosystem-friendly open solution for all. + +This rest of this tutorial includes: + +- Elaborates what the stable C layout looks like; +- Examples C codes that follow the stable C ABI. + +Stable C Layout +--------------- + +The ``universal_c_abi`` function uses a stable layout for all the input and output arguments. + +Layout of ``Any`` +~~~~~~~~~~~~~~~~~ + +TVM-FFI's ``Any`` is a fixed size (128-bit) tagged union that represents all supported types. + +- First 32 bits: represent the type index, indicating which type the value holds. By definition, it represents at most 2^32 different types. +- Next 32 bits: null padding, or reserved as special flags in rare cases. +- Last 64 bits: represent the value itself, which can be an 64-bit integer, floating pointer number, or pointer to a heap allocated object. Review Comment:  There's a typo in this line. "floating pointer number" should be "floating point number". ```suggestion - Last 64 bits: represent the value itself, which can be an 64-bit integer, floating point number, or pointer to a heap allocated object. ``` ########## examples/stable_c_abi/src/load.c: ########## @@ -0,0 +1,118 @@ +/* + * 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. + */ +// NOLINTBEGIN(modernize-deprecated-headers,modernize-use-nullptr,bugprone-assignment-in-if-condition,modernize-loop-convert) +// [main.begin] +// File: src/load.c +#include <stdio.h> +#include <tvm/ffi/c_api.h> +#include <tvm/ffi/extra/c_env_api.h> + +// Global functions are looked up during `Initialize` and deallocated during `Finalize` +// - global function: "ffi.Module.load_from_file.so" +static TVMFFIObjectHandle fn_load_module = NULL; +// - global function: "ffi.ModuleGetFunction" +static TVMFFIObjectHandle fn_get_function = NULL; + +int Run(DLTensor* x, DLTensor* y) { + int ret_code = 0; + TVMFFIAny call_args[3] = {}; + TVMFFIAny mod = {.type_index = kTVMFFINone, .v_obj = NULL}; + TVMFFIAny func = {.type_index = kTVMFFINone, .v_obj = NULL}; + + // Step 1. Load module + // Equivalent to: + // mod = tvm::ffi::Module::LoadFromFile("build/add_one_cpu.so") + call_args[0] = (TVMFFIAny){.type_index = kTVMFFIRawStr, .v_c_str = "build/add_one_cpu.so"}; + call_args[1] = (TVMFFIAny){.type_index = kTVMFFISmallStr, .v_int64 = 0}; + if ((ret_code = TVMFFIFunctionCall(fn_load_module, call_args, 2, &mod))) goto _RAII; + + // Step 2. Get function `add_one_cpu` from module + // Equivalent to: + // func = mod->GetFunction("add_one_cpu", /*query_imports=*/false).value() + call_args[0] = (TVMFFIAny){.type_index = mod.type_index, .v_obj = mod.v_obj}; + call_args[1] = (TVMFFIAny){.type_index = kTVMFFIRawStr, .v_c_str = "add_one_cpu"}; + call_args[2] = (TVMFFIAny){.type_index = kTVMFFIBool, .v_int64 = 0}; + if ((ret_code = TVMFFIFunctionCall(fn_get_function, call_args, 3, &func))) goto _RAII; + + // Step 3. Call function `add_one_cpu(x, y)` + // Equivalent to: + // func(x, y) + call_args[0] = (TVMFFIAny){.type_index = kTVMFFIDLTensorPtr, .v_ptr = x}; + call_args[1] = (TVMFFIAny){.type_index = kTVMFFIDLTensorPtr, .v_ptr = y}; + if ((ret_code = TVMFFIFunctionCall(func.v_ptr, call_args, 2, &((TVMFFIAny){})))) goto _RAII; + +_RAII: + if (mod.type_index >= kTVMFFIObject && mod.v_obj) TVMFFIObjectDecRef(mod.v_obj); + if (func.type_index >= kTVMFFIObject && func.v_obj) TVMFFIObjectDecRef(func.v_obj); + return ret_code; +} +// [main.end] + +/************* Auxiliary Logics *************/ + +// [aux.begin] +static inline int Initialize() { + int ret_code = 0; + TVMFFIByteArray name_load_module = {.data = "ffi.Module.load_from_file.so", .size = 28}; + TVMFFIByteArray name_get_function = {.data = "ffi.ModuleGetFunction", .size = 21}; + if ((ret_code = TVMFFIFunctionGetGlobal(&name_load_module, &fn_load_module))) return ret_code; + if ((ret_code = TVMFFIFunctionGetGlobal(&name_get_function, &fn_get_function))) return ret_code; + return 0; +} + +static inline void Finalize(int ret_code) { + TVMFFIObjectHandle err = NULL; + TVMFFIErrorCell* cell = NULL; + if (fn_load_module) TVMFFIObjectDecRef(fn_load_module); + if (fn_get_function) TVMFFIObjectDecRef(fn_get_function); + if (ret_code) { + TVMFFIErrorMoveFromRaised(&err); + cell = (TVMFFIErrorCell*)((char*)(err) + sizeof(TVMFFIObject)); + printf("%.*s: %.*s\n", (int)(cell->kind.size), cell->kind.data, (int)(cell->message.size), + cell->message.data); + } +} + +int main() { + Initialize(); + int ret_code = 0; + float x_data[5] = {1.0, 2.0, 3.0, 4.0, 5.0}; + float y_data[5] = {0.0, 0.0, 0.0, 0.0, 0.0}; + int64_t shape[1] = {5}; + int64_t strides[1] = {0}; Review Comment:  The `strides` for the `DLTensor` are initialized to `{0}`. For a 1D tensor, a stride of 0 means that all elements point to the same memory location, which is incorrect for this `add_one` operation. For a contiguous 1D tensor, the stride should be 1. Alternatively, you can set the `strides` pointer to `NULL` to indicate that the tensor is compact and contiguous, and the runtime will infer the correct strides. ```suggestion int64_t strides[1] = {1}; ``` ########## examples/stable_c_abi/src/load.c: ########## @@ -0,0 +1,118 @@ +/* + * 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. + */ +// NOLINTBEGIN(modernize-deprecated-headers,modernize-use-nullptr,bugprone-assignment-in-if-condition,modernize-loop-convert) +// [main.begin] +// File: src/load.c +#include <stdio.h> +#include <tvm/ffi/c_api.h> +#include <tvm/ffi/extra/c_env_api.h> + +// Global functions are looked up during `Initialize` and deallocated during `Finalize` +// - global function: "ffi.Module.load_from_file.so" +static TVMFFIObjectHandle fn_load_module = NULL; +// - global function: "ffi.ModuleGetFunction" +static TVMFFIObjectHandle fn_get_function = NULL; + +int Run(DLTensor* x, DLTensor* y) { + int ret_code = 0; + TVMFFIAny call_args[3] = {}; + TVMFFIAny mod = {.type_index = kTVMFFINone, .v_obj = NULL}; + TVMFFIAny func = {.type_index = kTVMFFINone, .v_obj = NULL}; + + // Step 1. Load module + // Equivalent to: + // mod = tvm::ffi::Module::LoadFromFile("build/add_one_cpu.so") + call_args[0] = (TVMFFIAny){.type_index = kTVMFFIRawStr, .v_c_str = "build/add_one_cpu.so"}; + call_args[1] = (TVMFFIAny){.type_index = kTVMFFISmallStr, .v_int64 = 0}; + if ((ret_code = TVMFFIFunctionCall(fn_load_module, call_args, 2, &mod))) goto _RAII; + + // Step 2. Get function `add_one_cpu` from module + // Equivalent to: + // func = mod->GetFunction("add_one_cpu", /*query_imports=*/false).value() + call_args[0] = (TVMFFIAny){.type_index = mod.type_index, .v_obj = mod.v_obj}; + call_args[1] = (TVMFFIAny){.type_index = kTVMFFIRawStr, .v_c_str = "add_one_cpu"}; + call_args[2] = (TVMFFIAny){.type_index = kTVMFFIBool, .v_int64 = 0}; + if ((ret_code = TVMFFIFunctionCall(fn_get_function, call_args, 3, &func))) goto _RAII; + + // Step 3. Call function `add_one_cpu(x, y)` + // Equivalent to: + // func(x, y) + call_args[0] = (TVMFFIAny){.type_index = kTVMFFIDLTensorPtr, .v_ptr = x}; + call_args[1] = (TVMFFIAny){.type_index = kTVMFFIDLTensorPtr, .v_ptr = y}; + if ((ret_code = TVMFFIFunctionCall(func.v_ptr, call_args, 2, &((TVMFFIAny){})))) goto _RAII; Review Comment:  Using a compound literal `&((TVMFFIAny){})` for the `result` argument is valid C99 but can be a bit obscure and less readable. For clarity, especially in an example file, it would be better to declare a `TVMFFIAny` variable, initialize it, and then pass its address. This makes the code more explicit about what is being passed. ```suggestion TVMFFIAny result_any = {.type_index = kTVMFFINone}; if ((ret_code = TVMFFIFunctionCall(func.v_ptr, call_args, 2, &result_any))) goto _RAII; ``` -- 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]
