gemini-code-assist[bot] commented on code in PR #19615:
URL: https://github.com/apache/tvm/pull/19615#discussion_r3305116275
##########
include/tvm/ir/attrs.h:
##########
@@ -160,8 +124,7 @@ class DictAttrs : public Attrs {
ffi::Optional<TObjectRef> GetAttr(
const std::string& attr_key,
ffi::Optional<TObjectRef> default_value =
ffi::Optional<TObjectRef>(std::nullopt)) const {
- if (!defined()) return default_value;
- const DictAttrsNode* node = this->as<DictAttrsNode>();
+ const DictAttrsNode* node = get();
Review Comment:
Removing the `defined()` check here is risky. Even though `DictAttrs` is
marked as `NOTNULLABLE`, instances of `DictAttrs` can still be null at runtime
(for example, if they are default-constructed, moved-from, or deserialized from
legacy/external IR where the attributes field was omitted or null).
Dereferencing `node` when it is null will cause a segmentation fault. We should
restore the safety check.
```suggestion
if (!defined()) return default_value;
const DictAttrsNode* node = get();
```
##########
include/tvm/ir/attrs.h:
##########
@@ -280,7 +196,7 @@ inline TFunc WithAttr(TFunc input, const std::string&
attr_key, Any attr_value)
using TNode = typename TFunc::ContainerType;
static_assert(TNode::_type_final, "Can only operate on the leaf nodes");
TNode* node = input.CopyOnWrite();
- node->attrs = WithAttr(std::move(node->attrs), attr_key, attr_value);
+ node->attrs.CopyOnWrite()->dict.Set(attr_key, std::move(attr_value));
Review Comment:
If `node->attrs` is null (which can happen if the function was deserialized
from a format where attributes were omitted), calling `CopyOnWrite()` on it
will cause a segmentation fault. We should defensively initialize `node->attrs`
if it is not defined.
```c
if (!node->attrs.defined()) {
node->attrs = DictAttrs();
}
node->attrs.CopyOnWrite()->dict.Set(attr_key, std::move(attr_value));
```
##########
include/tvm/ir/attrs.h:
##########
@@ -335,29 +253,10 @@ template <typename TFunc>
inline TFunc WithoutAttr(TFunc input, const std::string& attr_key) {
using TNode = typename TFunc::ContainerType;
static_assert(TNode::_type_final, "Can only operate on the leaf nodes");
-
TNode* node = input.CopyOnWrite();
- node->attrs = WithoutAttr(std::move(node->attrs), attr_key);
-
+ node->attrs.CopyOnWrite()->dict.erase(attr_key);
Review Comment:
If `node->attrs` is null, calling `CopyOnWrite()` on it will cause a
segmentation fault. We should check if `node->attrs` is defined before
attempting to erase from it.
```c
if (node->attrs.defined()) {
node->attrs.CopyOnWrite()->dict.erase(attr_key);
}
```
##########
include/tvm/ir/attrs.h:
##########
@@ -298,10 +214,12 @@ template <typename TFunc>
inline TFunc WithAttrs(TFunc input, ffi::Map<ffi::String, Any> attrs) {
using TNode = typename TFunc::ContainerType;
static_assert(TNode::_type_final, "Can only operate on the leaf nodes");
+ if (attrs.empty()) return input;
TNode* node = input.CopyOnWrite();
-
- node->attrs = WithAttrs(std::move(node->attrs), attrs);
-
+ auto* dict_node = node->attrs.CopyOnWrite();
Review Comment:
If `node->attrs` is null, calling `CopyOnWrite()` on it will cause a
segmentation fault. We should defensively initialize `node->attrs` if it is not
defined.
```suggestion
if (!node->attrs.defined()) {
node->attrs = DictAttrs();
}
auto* dict_node = node->attrs.CopyOnWrite();
```
##########
src/script/printer/ir/ir.cc:
##########
@@ -75,7 +75,7 @@ TVM_STATIC_IR_FUNCTOR(IRDocsifier, vtable)
(*f)->AddDispatchToken(d, "ir");
IdDoc module_doc = d->Define(mod, f(),
GetBindingName(d).value_or("Module"));
(*f)->global_infos = &mod->global_infos;
- if (mod->attrs.defined() && !mod->attrs->dict.empty()) {
+ if (!mod->attrs->dict.empty()) {
Review Comment:
If `mod->attrs` is null, calling `mod->attrs->dict` will cause a
segmentation fault. We should check if `mod->attrs` is defined before accessing
its dictionary.
```suggestion
if (mod->attrs.defined() && !mod->attrs->dict.empty()) {
```
##########
src/tirx/analysis/verify_tirx_well_formed.cc:
##########
@@ -251,8 +251,7 @@ bool VerifyTIRxWellFormed(const IRModule& mod, bool
assert_mode, bool device_fun
for (const auto& [gvar, base_func] : mod->functions) {
if (auto prim_func = base_func.as<PrimFunc>()) {
// s_tir=True PrimFuncs use s_tir semantics — defer to VerifyWellFormed.
- if (prim_func.value()->attrs.defined() &&
- prim_func.value()->attrs->dict.count(tvm::attr::kSTir)) {
+ if (prim_func.value()->attrs->dict.count(tvm::attr::kSTir)) {
Review Comment:
If `prim_func.value()->attrs` is null, calling
`prim_func.value()->attrs->dict` will cause a segmentation fault. We should
check if `attrs` is defined before accessing its dictionary.
```suggestion
if (prim_func.value()->attrs.defined() &&
prim_func.value()->attrs->dict.count(tvm::attr::kSTir)) {
```
##########
src/tirx/script/printer/function.cc:
##########
@@ -106,7 +106,7 @@ TVM_STATIC_IR_FUNCTOR(IRDocsifier, vtable)
if (d->cfg->syntax_sugar && CountVarOccurrence(func, var) == 2 &&
func->buffer_map.count(var)) {
tirx::Buffer buffer = func->buffer_map[var];
- bool s_tir = func->attrs.defined() &&
func->attrs->dict.count(tvm::attr::kSTir);
+ bool s_tir = func->attrs->dict.count(tvm::attr::kSTir);
Review Comment:
If `func->attrs` is null, calling `func->attrs->dict` will cause a
segmentation fault. We should check if `func->attrs` is defined before
accessing its dictionary.
```suggestion
bool s_tir = func->attrs.defined() &&
func->attrs->dict.count(tvm::attr::kSTir);
```
##########
src/tirx/script/printer/buffer.cc:
##########
@@ -193,7 +193,7 @@ ffi::Map<ffi::String, ExprDoc> BufferAttrs(tirx::Buffer
buffer, const AccessPath
for (const auto& f : d->frames) {
if (const auto* tir_f = f.as<TIRFrameNode>()) {
if (auto func = tir_f->tirx.as<tirx::PrimFuncNode>()) {
- if (func->attrs.defined() &&
func->attrs->dict.count(tvm::attr::kSTir)) {
+ if (func->attrs->dict.count(tvm::attr::kSTir)) {
Review Comment:
If `func->attrs` is null, calling `func->attrs->dict` will cause a
segmentation fault. We should check if `func->attrs` is defined before
accessing its dictionary.
```suggestion
if (func->attrs.defined() &&
func->attrs->dict.count(tvm::attr::kSTir)) {
```
##########
src/tirx/script/printer/function.cc:
##########
@@ -214,15 +214,15 @@ TVM_STATIC_IR_FUNCTOR(IRDocsifier, vtable)
ffi::Array<ffi::String, void> kwargs_keys;
ffi::Array<ExprDoc, void> kwargs_values;
// mark private if there is no global symbol
- if (!func->attrs.defined() ||
!func->attrs->dict.count(tvm::attr::kGlobalSymbol)) {
+ if (!func->attrs->dict.count(tvm::attr::kGlobalSymbol)) {
Review Comment:
If `func->attrs` is null, calling `func->attrs->dict` will cause a
segmentation fault. We should check if `func->attrs` is defined before
accessing its dictionary.
```suggestion
if (!func->attrs.defined() ||
!func->attrs->dict.count(tvm::attr::kGlobalSymbol)) {
```
##########
src/tirx/transform/split_host_device.cc:
##########
@@ -112,7 +112,7 @@ class HostDeviceSplitter : public StmtMutator {
device_func = WithAttrs(std::move(device_func), {{tvm::attr::kTarget,
device_target},
{tirx::attr::kNoAlias,
true},
{tirx::attr::kIsGlobalFunc, true}});
- if (cur_func_->attrs.defined() &&
cur_func_->attrs->dict.count(tvm::attr::kSTir)) {
+ if (cur_func_->attrs->dict.count(tvm::attr::kSTir)) {
Review Comment:
If `cur_func_->attrs` is null, calling `cur_func_->attrs->dict` will cause a
segmentation fault. We should check if `cur_func_->attrs` is defined before
accessing its dictionary.
```suggestion
if (cur_func_->attrs.defined() &&
cur_func_->attrs->dict.count(tvm::attr::kSTir)) {
```
##########
src/tirx/transform/ir_utils.cc:
##########
@@ -158,10 +158,6 @@ class IRConvertSSA final : public StmtExprMutator {
}();
auto attrs = [&]() -> DictAttrs {
- if (!func->attrs.defined()) {
- return DictAttrs();
- }
-
ffi::Map<ffi::String, ffi::Any> dict;
Review Comment:
If `func->attrs` is null, removing this check will cause a segmentation
fault when accessing `func->attrs->dict` later in the lambda. We should keep
the `defined()` check.
```c
if (!func->attrs.defined()) {
return DictAttrs();
}
ffi::Map<ffi::String, ffi::Any> dict;
```
##########
src/tirx/ir/function.cc:
##########
@@ -77,10 +77,6 @@ relax::StructInfo InferStructInfo(const PrimFunc& prim_func)
{
// Get the function type of a PrimFunc
PrimFunc::PrimFunc(ffi::Array<tirx::Var> params, Stmt body, Type ret_type,
ffi::Map<tirx::Var, Buffer> buffer_map, DictAttrs attrs,
Span span) {
- if (!attrs.defined()) {
- attrs = DictAttrs();
- }
-
if (!ret_type.defined()) {
Review Comment:
If `attrs` is passed as an undefined `DictAttrs`, leaving it uninitialized
will result in a null `attrs` field in the `PrimFunc` node, leading to
downstream segmentation faults. We should keep the fallback initialization.
```c
if (!attrs.defined()) {
attrs = DictAttrs();
}
if (!ret_type.defined()) {
```
##########
src/relax/ir/dataflow_matcher.cc:
##########
@@ -209,7 +209,7 @@ bool DFPatternMatcher::VisitDFPattern_(const
AttrPatternNode* attr_pattern, cons
} else if (auto* op = expr.as<FunctionNode>()) {
matches = true;
for (auto kv : attributes) {
- if (matches && op->attrs.defined() && op->attrs->dict.count(kv.first)) {
+ if (matches && op->attrs->dict.count(kv.first)) {
Review Comment:
If `op->attrs` is null (which can happen if the function was deserialized
without attributes or created via legacy APIs), dereferencing it via
`op->attrs->dict` will cause a segmentation fault. It is safer to keep the
`defined()` check.
```suggestion
if (matches && op->attrs.defined() && op->attrs->dict.count(kv.first))
{
```
##########
src/target/cuda/codegen_cuda.cc:
##########
@@ -210,7 +210,7 @@ void CodeGenCUDA::PrintExtraAttrs(const PrimFunc& f,
std::ostream& os) {
extractor(f->body);
// Also check PrimFunc attrs for persistent kernel (decorator-level)
bool is_persistent = extractor.is_persistent_kernel;
- if (!is_persistent && f->attrs.defined() &&
f->attrs->dict.count(tirx::attr::kPersistentKernel)) {
+ if (!is_persistent && f->attrs->dict.count(tirx::attr::kPersistentKernel)) {
Review Comment:
If `f->attrs` is null, calling `f->attrs->dict` will cause a segmentation
fault. We should check if `f->attrs` is defined before accessing its dictionary.
```suggestion
if (!is_persistent && f->attrs.defined() &&
f->attrs->dict.count(tirx::attr::kPersistentKernel)) {
```
##########
src/relax/ir/expr.cc:
##########
@@ -542,10 +542,6 @@ TVM_FFI_STATIC_INIT_BLOCK() {
Function::Function(ffi::Array<Var> params, Expr body,
ffi::Optional<StructInfo> ret_struct_info,
bool is_pure, DictAttrs attrs, Span span) {
- if (!attrs.defined()) {
- attrs = DictAttrs();
- }
-
// Set the function type.
Review Comment:
If `attrs` is passed as an undefined `DictAttrs` (which can happen if a
caller passes a default-constructed or null `DictAttrs`), leaving it
uninitialized will result in a null `attrs` field in the `Function` node,
leading to downstream segmentation faults. We should keep the fallback
initialization.
```suggestion
if (!attrs.defined()) {
attrs = DictAttrs();
}
// Set the function type.
```
##########
src/tirx/script/printer/function.cc:
##########
@@ -120,7 +120,7 @@ TVM_STATIC_IR_FUNCTOR(IRDocsifier, vtable)
args.push_back(AssignDoc(DefineVar(var, *f, d), std::nullopt, a));
}
// Step 2. Handle `func->attrs`
- if (func->attrs.defined() && !func->attrs->dict.empty()) {
+ if (!func->attrs->dict.empty()) {
Review Comment:
If `func->attrs` is null, calling `func->attrs->dict` will cause a
segmentation fault. We should check if `func->attrs` is defined before
accessing its dictionary.
```suggestion
if (func->attrs.defined() && !func->attrs->dict.empty()) {
```
--
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]
