gemini-code-assist[bot] commented on code in PR #335:
URL: https://github.com/apache/tvm-ffi/pull/335#discussion_r2612651370


##########
docs/guides/kernel_library_guide.rst:
##########
@@ -94,6 +94,14 @@ However, the tensors allocated by 
:cpp:func:`tvm::ffi::Tensor::FromNDAlloc` only
 
 But in the scenarios of linked runtime libraries and c++ applications, the 
libraries alive globally throughout the entire lifetime of the process. So 
:cpp:func:`tvm::ffi::Tensor::FromNDAlloc` works well in these scenarios without 
the use-after-delete issue above. Otherwise, in general, 
:cpp:func:`tvm::ffi::Tensor::FromEnvAlloc` is free of this issue, which is more 
**recommended** in practice.
 
+
+FromNDAllocStrided
+^^^^^^^^^^^^^^^^^^
+
+:cpp:func:`tvm::ffi::Tensor::FromNDAllocStrided` can be used to create a 
tensor with a custom memory allocator and strided layout(e.g. column major 
layout).

Review Comment:
   ![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg)
   
   There is a minor typo in the documentation. A space is missing before the 
opening parenthesis, which affects readability.
   
   ```suggestion
   :cpp:func:`tvm::ffi::Tensor::FromNDAllocStrided` can be used to create a 
tensor with a custom memory allocator and strided layout (e.g. column major 
layout).
   ```



##########
include/tvm/ffi/container/tensor.h:
##########
@@ -348,6 +361,38 @@ class Tensor : public ObjectRef {
    * \return True if the Tensor data is aligned to the given alignment, false 
otherwise.
    */
   bool IsAligned(size_t alignment) const { return tvm::ffi::IsAligned(*get(), 
alignment); }
+
+  /*
+   * \brief Create a new Tensor as a strided view of the current Tensor.
+   * \param shape The shape of the new Tensor.
+   * \param strides The strides of the new Tensor.
+   * \param element_offset The element offset of the new Tensor in the unit of 
dtype elements.
+   * \return The new Tensor.
+   * \note element_offset is in the unit of dtype elements not bytes.
+   */
+  Tensor as_strided(ShapeView shape, ShapeView strides,
+                    std::optional<int64_t> element_offset = std::nullopt) 
const {
+    DLTensor prototype;
+    prototype = *static_cast<const DLTensor*>(get());
+    prototype.shape = const_cast<int64_t*>(shape.data());
+    prototype.ndim = static_cast<int>(shape.size());
+    prototype.strides = const_cast<int64_t*>(strides.data());
+    prototype.byte_offset =
+        GetDataSize(static_cast<size_t>(element_offset.value_or(0)), 
prototype.dtype);

Review Comment:
   ![critical](https://www.gstatic.com/codereviewagent/critical.svg)
   
   The `byte_offset` of the new strided view is being overwritten instead of 
being accumulated. If the original tensor already has a non-zero `byte_offset` 
(e.g., it's a view of another tensor), this offset will be lost. The new offset 
calculated from `element_offset` should be added to the existing `byte_offset` 
to ensure the view is correct.
   
   ```c
       prototype.byte_offset +=
           GetDataSize(static_cast<size_t>(element_offset.value_or(0)), 
prototype.dtype);
   ```



-- 
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]

Reply via email to