gemini-code-assist[bot] commented on code in PR #330:
URL: https://github.com/apache/tvm-ffi/pull/330#discussion_r2608873505
##########
python/tvm_ffi/utils/_build_optional_torch_c_dlpack.py:
##########
@@ -543,7 +543,13 @@
.dtype(at::toScalarType(prototype->dtype))
.device(at::getATenDeviceForDLPackv1(prototype->device.device_type,
prototype->device.device_id));
- at::Tensor tensor = at::empty(shape, options);
+ at::Tensor tensor;
+ if (prototype->strides != nullptr) {
+ at::IntArrayRef stride(prototype->strides);
+ at::Tensor tensor = at::empty_strided(shape, stride, options);
+ } else {
+ tensor = at::empty(shape, options);
+ }
Review Comment:
This block has two critical issues:
1. **Variable Shadowing**: On line 549, `at::Tensor tensor` is re-declared,
shadowing the `tensor` variable from the outer scope (line 546). This means the
outer `tensor` will be uninitialized if the `if` branch is taken, leading to
undefined behavior on line 553.
2. **Incorrect Constructor Usage**: On line 548, `at::IntArrayRef
stride(prototype->strides)` is incorrect. The `at::IntArrayRef` constructor
requires both a pointer and the size. It should be `at::IntArrayRef
stride(prototype->strides, prototype->ndim);`.
Here is the corrected code:
```suggestion
at::Tensor tensor;
if (prototype->strides != nullptr) {
at::IntArrayRef stride(prototype->strides, prototype->ndim);
tensor = at::empty_strided(shape, stride, options);
} else {
tensor = at::empty(shape, options);
}
```
##########
include/tvm/ffi/container/tensor.h:
##########
@@ -442,11 +442,42 @@ class Tensor : public ObjectRef {
*/
static Tensor FromEnvAlloc(int (*env_alloc)(DLTensor*, TVMFFIObjectHandle*),
ffi::ShapeView shape,
DLDataType dtype, DLDevice device) {
+ return FromEnvAlloc(env_alloc, shape, ShapeView(), dtype, device);
+ }
+ /*!
+ * \brief Create a Tensor from the TVMFFIEnvTensorAlloc API
+ *
+ * This function can be used together with
TVMFFIEnvSetDLPackManagedTensorAllocator
+ * in the extra/c_env_api.h to create a Tensor from the thread-local
environment allocator.
+ * We explicitly pass TVMFFIEnvTensorAlloc to maintain explicit dependency
on extra/c_env_api.h
+ *
+ * \code
+ *
+ * ffi::Tensor tensor = ffi::Tensor::FromEnvAlloc(
+ * TVMFFIEnvTensorAlloc, shape, strides, dtype, device
+ * );
+ *
+ * \endcode
+ *
+ * \param env_alloc TVMFFIEnvTensorAlloc function pointer.
+ * \param shape The shape of the Tensor.
+ * \param stride The stride of the Tensor.
+ * \param dtype The data type of the Tensor.
+ * \param device The device of the Tensor.
+ * \return The created Tensor.
+ *
+ * \sa TVMFFIEnvTensorAlloc
+ */
+ static Tensor FromEnvAlloc(int (*env_alloc)(DLTensor*, TVMFFIObjectHandle*),
ffi::ShapeView shape,
+ ffi::ShapeView stride, DLDataType dtype, DLDevice
device) {
Review Comment:
For consistency with `shape` and the documentation example, the parameter
`stride` should be renamed to `strides` as it represents a view over multiple
stride values. You'll also need to update its usage within this function and
its documentation on line 464.
```c
static Tensor FromEnvAlloc(int (*env_alloc)(DLTensor*,
TVMFFIObjectHandle*), ffi::ShapeView shape,
ffi::ShapeView strides, DLDataType dtype,
DLDevice device) {
```
--
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]
