gemini-code-assist[bot] commented on code in PR #18784:
URL: https://github.com/apache/tvm/pull/18784#discussion_r2809873473
##########
src/runtime/contrib/clml/clml_runtime.cc:
##########
@@ -1079,9 +1085,9 @@ class CLMLRuntime : public JSONRuntimeBase {
layer->function.push_back(op);
} else {
int bn_index = has_bias ? 3 : 2;
- int axis =
std::stoi(node.GetAttr<std::vector<std::string>>("batchnorm")[0]);
+ int axis =
std::stoi(node.GetAttr<ffi::Array<ffi::String>>("batchnorm")[0]);
auto bn_dims = GetTensorDims(nodes_[inputs[bn_index].id_]);
- float epsilon =
std::stof(node.GetAttr<std::vector<std::string>>("batchnorm")[1]);
+ float epsilon =
std::stof(node.GetAttr<ffi::Array<ffi::String>>("batchnorm")[1]);
Review Comment:
The `batchnorm` attribute is being retrieved from the node twice, which is
inefficient. It would be better to get the attribute once and store it in a
local variable.
```c
auto batchnorm_attr =
node.GetAttr<ffi::Array<ffi::String>>("batchnorm");
int axis = std::stoi(batchnorm_attr[0]);
auto bn_dims = GetTensorDims(nodes_[inputs[bn_index].id_]);
float epsilon = std::stof(batchnorm_attr[1]);
```
##########
src/relax/backend/contrib/codegen_json/codegen_json.h:
##########
@@ -59,83 +58,102 @@ class OpAttrExtractor {
public:
explicit OpAttrExtractor(JSONGraphObjectPtr node) : node_(node) {}
- template <typename T = double, typename =
std::enable_if_t<std::is_floating_point<T>::value>>
- std::string Fp2String(const T value) {
- std::ostringstream out;
- out.precision(std::numeric_limits<T>::max_digits10);
- out << value;
- return out.str();
- }
-
- void SetNodeAttr(const char* key, const std::vector<std::string>& value) {
- std::vector<std::any> attr;
- attr.emplace_back(value);
- node_->SetAttr(key, attr);
- }
+ void SetNodeAttr(const char* key, ffi::Any value) { node_->SetAttr(key,
std::move(value)); }
- void Visit(const char* key, double* value) { SetNodeAttr(key,
{Fp2String(*value)}); }
+ void Visit(const char* key, double* value) { SetNodeAttr(key, *value); }
- void Visit(const char* key, int64_t* value) { SetNodeAttr(key,
{std::to_string(*value)}); }
+ void Visit(const char* key, int64_t* value) { SetNodeAttr(key, *value); }
- void Visit(const char* key, uint64_t* value) { SetNodeAttr(key,
{std::to_string(*value)}); }
+ void Visit(const char* key, uint64_t* value) { SetNodeAttr(key,
static_cast<int64_t>(*value)); }
- void Visit(const char* key, int* value) { SetNodeAttr(key,
{std::to_string(*value)}); }
+ void Visit(const char* key, int* value) { SetNodeAttr(key,
static_cast<int64_t>(*value)); }
- void Visit(const char* key, bool* value) { SetNodeAttr(key,
{std::to_string(*value)}); }
+ void Visit(const char* key, bool* value) { SetNodeAttr(key,
static_cast<int64_t>(*value)); }
- void Visit(const char* key, std::string* value) { SetNodeAttr(key,
{*value}); }
+ void Visit(const char* key, std::string* value) { SetNodeAttr(key,
ffi::String(*value)); }
void Visit(const char* key, ffi::Optional<double>* value) {
if (value->has_value()) {
- SetNodeAttr(key, {Fp2String(value->value())});
+ SetNodeAttr(key, value->value());
} else {
- SetNodeAttr(key, {""});
+ SetNodeAttr(key, ffi::String(""));
}
}
void Visit(const char* key, ffi::Optional<int64_t>* value) {
if (value->has_value()) {
- SetNodeAttr(key, {std::to_string(value->value())});
+ SetNodeAttr(key, value->value());
} else {
- SetNodeAttr(key, {""});
+ SetNodeAttr(key, ffi::String(""));
}
}
void Visit(const char* key, DataType* value) {
if (!value->is_void()) {
- SetNodeAttr(key, {runtime::DLDataTypeToString(*value)});
+ SetNodeAttr(key, ffi::String(runtime::DLDataTypeToString(*value)));
} else {
- SetNodeAttr(key, {""});
+ SetNodeAttr(key, ffi::String(""));
}
}
void Visit(const char* key, ffi::Any* value) {
if (const auto* an = (*value).as<ffi::ArrayObj>()) {
- std::vector<std::string> attr;
+ // Determine element type from first element and build typed array
+ if (an->size() == 0) {
+ SetNodeAttr(key, ffi::Array<int64_t>{});
+ return;
+ }
+ // Try int64_t array (covers IntImm too)
+ bool all_int = true;
+ bool all_float = true;
for (size_t i = 0; i < an->size(); ++i) {
- if (auto opt_int = (*an)[i].try_cast<int64_t>()) {
- attr.push_back(std::to_string(opt_int.value()));
- } else if (const auto* im = (*an)[i].as<IntImmNode>()) {
- attr.push_back(std::to_string(im->value));
- } else if (auto opt_float = (*an)[i].try_cast<double>()) {
- attr.push_back(Fp2String(opt_float.value()));
- } else if (const auto* fm = (*an)[i].as<FloatImmNode>()) {
- attr.push_back(Fp2String(fm->value));
- } else if (auto opt_str = (*an)[i].as<ffi::String>()) {
- attr.push_back(*opt_str);
- } else {
- LOG(FATAL) << "Not supported type: " << (*an)[i].GetTypeKey();
+ if (!(*an)[i].try_cast<int64_t>() && !(*an)[i].as<IntImmNode>()) {
+ all_int = false;
+ }
+ if (!(*an)[i].try_cast<double>() && !(*an)[i].as<FloatImmNode>()) {
+ all_float = false;
+ }
+ }
Review Comment:
The logic for determining the array type is flawed. For an array containing
mixed integer and floating-point types (e.g., `[1, 2.0]`), both `all_int` and
`all_float` will become `false`. This will cause the code to fall back to the
string array case, which will then fail with a `LOG(FATAL)` because the numeric
elements cannot be cast to `ffi::String`.
The logic should be updated to correctly handle mixed numeric types by
promoting them to a common type (e.g., `double`).
```c
bool can_be_int = true;
bool can_be_double = true;
for (size_t i = 0; i < an->size(); ++i) {
const auto& elem = (*an)[i];
bool is_int = elem.try_cast<int64_t>() || elem.as<IntImmNode>();
bool is_float = elem.try_cast<double>() || elem.as<FloatImmNode>();
if (!is_int) {
can_be_int = false;
}
if (!is_int && !is_float) {
can_be_double = false;
break;
}
}
if (can_be_int) {
ffi::Array<int64_t> attr;
for (size_t i = 0; i < an->size(); ++i) {
if (auto opt_int = (*an)[i].try_cast<int64_t>()) {
attr.push_back(opt_int.value());
} else if (const auto* im = (*an)[i].as<IntImmNode>()) {
attr.push_back(im->value);
}
}
SetNodeAttr(key, std::move(attr));
} else if (can_be_double) {
ffi::Array<double> attr;
for (size_t i = 0; i < an->size(); ++i) {
const auto& elem = (*an)[i];
if (auto opt_float = elem.try_cast<double>()) {
attr.push_back(opt_float.value());
} else if (const auto* fm = elem.as<FloatImmNode>()) {
attr.push_back(fm->value);
} else if (auto opt_int = elem.try_cast<int64_t>()) {
attr.push_back(static_cast<double>(opt_int.value()));
} else if (const auto* im = elem.as<IntImmNode>()) {
attr.push_back(static_cast<double>(im->value));
}
}
SetNodeAttr(key, std::move(attr));
```
--
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]
