gemini-code-assist[bot] commented on code in PR #107:
URL: https://github.com/apache/tvm-ffi/pull/107#discussion_r2426325318
##########
docs/guides/compiler_integration.md:
##########
@@ -86,7 +86,8 @@ Some of the key takeaways include:
- Prefix the symbol with `__tvm_ffi_`
- Call {cpp:func}`TVMFFIEnvGetStream` to get the current environment stream
-- Use return value for error handling, set error via
{cpp:func}`TVMFFIErrorSetRaisedFromCStr`.
+- Use return value for error handling, set error via
{cpp:func}`TVMFFIErrorSetRaisedFromCStr`
+ or {cpp::func}`TVMFFIErrorSetRaisedFromCStrParts`.
Review Comment:
There's a small typo in the Sphinx role here. It should be `{cpp:func}`
instead of `{cpp::func}` to be rendered correctly.
```suggestion
or {cpp:func}`TVMFFIErrorSetRaisedFromCStrParts`.
```
##########
include/tvm/ffi/c_api.h:
##########
@@ -566,13 +567,39 @@ TVM_FFI_DLL void
TVMFFIErrorMoveFromRaised(TVMFFIObjectHandle* result);
TVM_FFI_DLL void TVMFFIErrorSetRaised(TVMFFIObjectHandle error);
/*!
- * \brief Set a raised error in TLS, which can be fetched by
TVMFFIMoveFromRaised.
+ * \brief Set a raised error in TLS, which can be fetched by
TVMFFIErrorMoveFromRaised.
* \param kind The kind of the error.
* \param message The error message.
* \note This is a convenient method for the C API side to set an error
directly from a string.
*/
TVM_FFI_DLL void TVMFFIErrorSetRaisedFromCStr(const char* kind, const char*
message);
+/*!
+ * \brief Set a raised error in TLS, which can be fetched by
TVMFFIErrorMoveFromRaised.
+ *
+ * Rationale: This function can be used by compilers to create error messages
by
+ * concatenating multiple parts of the error message, which can reduce the
+ * storage size for common parts such as function signatures.
+ *
+ * For example, the following are possible error messages from a kernel DSL
+ *
+ * - Argument 1 mismatch in `matmul(x: Tensor, y: Tensor, z: Tensor)`, dtype
mismatch
+ * - Argument 2 mismatch in `matmul(x: Tensor, y: Tensor, z: Tensor)`,
shape[0] mismatch
+ * - Argument 2 mismatch in `matmul(x: Tensor, y: Tensor, z: Tensor)`,
shape[1] mismatch
+ *
+ * Storing each part of the error message as a separate global string can
cause quite
+ * a bit of duplication, especially considering the kinds of error reports we
may have.
+ * Instead, compilers can store error messages in parts, where items like
+ * `matmul(x: Tensor, y: Tensor)` can be reused across multiple error messages.
Review Comment:
There's a minor inconsistency in the documentation. The example function
signature mentioned here, `matmul(x: Tensor, y: Tensor)`, differs from the one
used in the error message examples on lines 586-588, which is `matmul(x:
Tensor, y: Tensor, z: Tensor)`. Aligning them would improve clarity.
```c
* `matmul(x: Tensor, y: Tensor, z: Tensor)` can be reused across multiple
error messages.
```
##########
src/ffi/error.cc:
##########
@@ -38,6 +38,18 @@ class SafeCallContext {
last_error_ =
details::ObjectUnsafe::ObjectPtrFromObjectRef<ErrorObj>(std::move(error));
}
+ void SetRaisedByCstrParts(const char* kind, const char** message_parts,
int32_t num_parts,
+ const TVMFFIByteArray* backtrace) {
+ std::string message;
+ for (int i = 0; i < num_parts; i++) {
+ if (message_parts[i] != nullptr) {
+ message += message_parts[i];
+ }
+ }
Review Comment:
Concatenating strings in a loop using `operator+=` can lead to multiple
reallocations and be inefficient, especially if there are many parts. A more
performant approach is to pre-calculate the total required size, reserve the
memory for the `std::string` once, and then append the parts. This avoids
intermediate reallocations.
```c
std::string message;
size_t total_len = 0;
for (int i = 0; i < num_parts; ++i) {
if (message_parts[i] != nullptr) {
total_len += strlen(message_parts[i]);
}
}
message.reserve(total_len);
for (int i = 0; i < num_parts; ++i) {
if (message_parts[i] != nullptr) {
message.append(message_parts[i]);
}
}
```
--
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]
