gemini-code-assist[bot] commented on code in PR #402: URL: https://github.com/apache/tvm-ffi/pull/402#discussion_r2680809673
########## docs/concepts/abi_overview.rst: ########## @@ -0,0 +1,466 @@ +.. 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. + +ABI Overview +============ + +.. hint:: + + Authoritative ABI specifications are defined in + + - C header `tvm/ffi/c_api.h <https://github.com/apache/tvm-ffi/blob/main/include/tvm/ffi/c_api.h>`_, which contains the core ABI, and + - C header `tvm/ffi/extra/c_env_api.h <https://github.com/apache/tvm-ffi/blob/main/include/tvm/ffi/extra/c_env_api.h>`_, which contains extra support features. + +Overall, TVM-FFI ABI is designed is around the following key principles: + +- **Minimal and efficient**. Keep things simple when possible and bring close-to-metal efficiency. +- **Stability guarantee**. The ABI is stable across compiler versions and independent of host languages or frameworks. +- **Expressive for machine learning** TBA. Review Comment:  This principle is marked as 'TBA'. Please either fill in the details or remove this point to avoid shipping incomplete documentation. ########## examples/abi_overview/example_code.c: ########## @@ -0,0 +1,185 @@ +/* + * 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) +#include <assert.h> +#include <dlpack/dlpack.h> +#include <tvm/ffi/c_api.h> + +int IS_OWNING_ANY = 1; + +// [Any_AnyView.FromInt_Float.begin] +TVMFFIAny Any_AnyView_FromInt(int64_t value) { + TVMFFIAny any; + any.type_index = kTVMFFIInt; + any.zero_padding = 0; + any.v_int64 = value; + return any; +} + +TVMFFIAny Any_AnyView_FromFloat(double value) { + TVMFFIAny any; + any.type_index = kTVMFFIFloat; + any.zero_padding = 0; + any.v_float64 = value; + return any; +} +// [Any_AnyView.FromInt_Float.end] + +// [Any_AnyView.FromObjectPtr.begin] +TVMFFIAny Any_AnyView_FromObjectPtr(TVMFFIObject* obj) { + TVMFFIAny any; + assert(obj != NULL); + any.type_index = kTVMFFIObject; + any.zero_padding = 0; + any.v_obj = obj; + // Increment refcount if it's Any (owning) instead of AnyView (borrowing) + if (IS_OWNING_ANY) { + TVMFFIObjectIncRef(obj); + } + return any; +} +// [Any_AnyView.FromObjectPtr.end] + +// [Any_AnyView.Destroy.begin] +void Any_AnyView_Destroy(TVMFFIAny* any) { + if (IS_OWNING_ANY) { + // Checks if `any` holds a heap-allocated object, + // and if so, decrements the reference count + if (any->type_index >= kTVMFFIStaticObjectBegin) { + TVMFFIObjectDecRef(any->v_obj); + } + } + *any = (TVMFFIAny){0}; // Clears the `any` struct +} +// [Any_AnyView.Destroy.end] + +// [Any_AnyView.GetInt_Float.begin] +int64_t Any_AnyView_GetInt(const TVMFFIAny* any) { + if (any->type_index == kTVMFFIInt || any->type_index == kTVMFFIBool) { + return any->v_int64; + } else if (any->type_index == kTVMFFIFloat) { + return (int64_t)(any->v_float64); + } + assert(0); // FAILED to read int + return 0; +} + +double Any_AnyView_GetFloat(const TVMFFIAny* any) { + if (any->type_index == kTVMFFIInt || any->type_index == kTVMFFIFloat) { + return (double)(any->v_int64); + } else if (any->type_index == kTVMFFIFloat) { + return any->v_float64; + } Review Comment:  There is a logical error in this function. If `any->type_index` is `kTVMFFIFloat`, the first `if` condition is met, and the function incorrectly returns `(double)(any->v_int64)`. The `else if` block is unreachable. You should separate the checks for `kTVMFFIInt` and `kTVMFFIFloat`. ```suggestion if (any->type_index == kTVMFFIInt) { return (double)(any->v_int64); } else if (any->type_index == kTVMFFIFloat) { return any->v_float64; } ``` ########## docs/concepts/abi_overview.rst: ########## @@ -0,0 +1,466 @@ +.. 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. + +ABI Overview +============ + +.. hint:: + + Authoritative ABI specifications are defined in + + - C header `tvm/ffi/c_api.h <https://github.com/apache/tvm-ffi/blob/main/include/tvm/ffi/c_api.h>`_, which contains the core ABI, and + - C header `tvm/ffi/extra/c_env_api.h <https://github.com/apache/tvm-ffi/blob/main/include/tvm/ffi/extra/c_env_api.h>`_, which contains extra support features. + +Overall, TVM-FFI ABI is designed is around the following key principles: + +- **Minimal and efficient**. Keep things simple when possible and bring close-to-metal efficiency. +- **Stability guarantee**. The ABI is stable across compiler versions and independent of host languages or frameworks. +- **Expressive for machine learning** TBA. +- **Extensible**. The ABI is extensible to support new types and features. + +This tutorial covers common concepts and usage patterns with TVM-FFI's ABI with low-level C code for precise reference. + +Any and AnyView +--------------- + +.. seealso:: + + :doc:`any` for :cpp:class:`~tvm::ffi::Any` and :cpp:class:`~tvm::ffi::AnyView` usage patterns. + +At the core of TVM-FFI is :cpp:class:`TVMFFIAny`, a 16-byte tagged union that can hold any value +recognized by the FFI system. It enables type-erased value passing across language boundaries. + +.. dropdown:: C ABI Reference: :cpp:class:`TVMFFIAny` + :icon: code + + .. literalinclude:: ../../include/tvm/ffi/c_api.h + :language: c + :start-after: [TVMFFIAny.begin] + :end-before: [TVMFFIAny.end] + :caption: tvm/ffi/c_api.h + +**Ownership**. :cpp:class:`TVMFFIAny` has two variants with exact same layout but different :ref:`ownership semantics <any-ownership>`: + +- Managed owning :cpp:class:`tvm::ffi::Any` +- Unmanaged borrowing :cpp:class:`tvm::ffi::AnyView` + +.. note:: + To switch a borrowing :cpp:class:`~tvm::ffi::AnyView` to an owning :cpp:class:`~tvm::ffi::Any`, use :cpp:func:`TVMFFIAnyViewToOwnedAny`. + +**Runtime Type Index**. The ``type_index`` field identifies what kind of value is stored. + +- :ref:`Atomic POD types <any-atomic-types>`, where ``type_index`` < :cpp:enumerator:`kTVMFFIStaticObjectBegin <TVMFFITypeIndex::kTVMFFIStaticObjectBegin>`. + They are stored in-place in payload union, without heap allocation or reference counting. +- :ref:`Object types <any-heap-allocated-objects>`, where ``type_index`` >= :cpp:enumerator:`kTVMFFIStaticObjectBegin <TVMFFITypeIndex::kTVMFFIStaticObjectBegin>`. + They are stored in a heap-allocated, reference-counted TVM-FFI object. + +.. important:: + Type index system in TVM-FFI does not rely on C++ features such as RTTI. + + +Construct Any +~~~~~~~~~~~~~ + +**Constructing from atomic POD types**. The C code below constructs an :cpp:class:`TVMFFIAny` from an integer: + + +.. literalinclude:: ../../examples/abi_overview/example_code.c + :language: c + :start-after: [Any_AnyView.FromInt_Float.begin] + :end-before: [Any_AnyView.FromInt_Float.end] + +Essentially, properly assign ``type_index`` from :cpp:enum:`TVMFFITypeIndex`, and then set the payload field accordingly. Review Comment:  The reference to `TVMFFITypeIndex` is not a link. To make it a link and be consistent with other references in this document, you should use the explicit target syntax. ```suggestion Essentially, properly assign ``type_index`` from :cpp:enum:`TVMFFITypeIndex <TVMFFITypeIndex>`, and then set the payload field accordingly. ``` ########## examples/abi_overview/example_code.c: ########## @@ -0,0 +1,185 @@ +/* + * 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) +#include <assert.h> +#include <dlpack/dlpack.h> +#include <tvm/ffi/c_api.h> + +int IS_OWNING_ANY = 1; Review Comment:  This global variable is used to control behavior in example functions. It's better to make it `static` to limit its scope to this file. This prevents potential name clashes if other files are added to this example directory. ```suggestion static int IS_OWNING_ANY = 1; ``` ########## docs/concepts/abi_overview.rst: ########## @@ -0,0 +1,466 @@ +.. 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. + +ABI Overview +============ + +.. hint:: + + Authoritative ABI specifications are defined in + + - C header `tvm/ffi/c_api.h <https://github.com/apache/tvm-ffi/blob/main/include/tvm/ffi/c_api.h>`_, which contains the core ABI, and + - C header `tvm/ffi/extra/c_env_api.h <https://github.com/apache/tvm-ffi/blob/main/include/tvm/ffi/extra/c_env_api.h>`_, which contains extra support features. + +Overall, TVM-FFI ABI is designed is around the following key principles: Review Comment:  There's a minor grammatical error here. It should be "is designed around" instead of "is designed is around". ```suggestion Overall, TVM-FFI ABI is designed around the following key principles: ``` -- 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]
