You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/07/19 06:23:59 UTC
[GitHub] [incubator-tvm] hanzz2007 opened a new issue #6093: [BUG] Exception may be throwed when attrs is constructed
hanzz2007 opened a new issue #6093:
URL: https://github.com/apache/incubator-tvm/issues/6093
```C++
template <typename FFind>
class AttrInitVisitor {
public:
// Counter of number of matched attributes during visit.
// This is used to decide if there is additional unmatched attributes.
size_t hit_count_{0};
// constructor
AttrInitVisitor(const char* type_key, FFind ffind) : type_key_(type_key), ffind_(ffind) {}
template <typename T>
AttrInitEntry<T> operator()(const char* key, T* value) {
TVMArgValue val;
AttrInitEntry<T> opt;
opt.type_key_ = type_key_;
opt.key_ = key;
opt.value_ = value;
if (ffind_(key, &val)) {
SetValue(value, val);
opt.value_missing_ = false;
++hit_count_;
} else {
opt.value_missing_ = true;
}
return opt;
// for compilers without RVO optimization, the opt.~AttrInitEntry<T>() will be called when function exit
// An exception will be throwed if opt.value_missing_ is ture in destructor
}
```
When the AttrsNode is first constructed
```C++
template <typename TAttrs>
inline TAttrs AttrsWithDefaultValues() {
static_assert(std::is_base_of<Attrs, TAttrs>::value, "Can only take attr nodes");
auto n = make_object<typename TAttrs::ContainerType>();
// Note here, will cause a problem
n->InitByPackedArgs(runtime::TVMArgs(nullptr, nullptr, 0), false);
return TAttrs(n);
}
```
Maybe we could fix it via the following codes
https://gist.github.com/hanzz2007/803b28a7915c31686cffce0ac6450b73
https://gist.github.com/hanzz2007/e42b14b51c8c9c765cca107867e64d49
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on issue #6093: [BUG] Exception may be throwed when attrs is constructed
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #6093:
URL: https://github.com/apache/incubator-tvm/issues/6093#issuecomment-661944703
I see, would it be suffice to force move in the return statement?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] hanzz2007 commented on issue #6093: [BUG] Exception may be throwed when attrs is constructed
Posted by GitBox <gi...@apache.org>.
hanzz2007 commented on issue #6093:
URL: https://github.com/apache/incubator-tvm/issues/6093#issuecomment-662554484
Yes, move can solve. And we should add a default constructor and a rvref constructor to AttrInitEntry
```c++
template <typename T>
struct AttrInitEntry {
// The attributes
using TSelf = AttrInitEntry<T>;
// The type key
const char* type_key_;
// field name
const char* key_;
// internal value.
T* value_;
// whether the value is missing.
bool value_missing_{true};
AttrInitEntry() = default;
AttrInitEntry(AttrInitEntry&& other) {
type_key_ = other.type_key_;
key_ = other.key_;
value_ = other.value_;
value_missing_ = other.value_missing_;
other.value_missing_ = false; // note! avoid throw
}
// If the value is still missing in destruction time throw an error.
~AttrInitEntry() DMLC_THROW_EXCEPTION {
if (value_missing_) {
std::ostringstream os;
os << type_key_ << ": Cannot find required field \'" << key_ << "\' during initialization";
throw AttrError(os.str());
}
}
// override fields.
// This function sets the lower bound of the attribute
TSelf& set_lower_bound(DMLC_ATTRIBUTE_UNUSED const T& begin) {
if (this->value_missing_) return *this;
const T& val = *value_;
if (begin > val) {
std::ostringstream os;
os << type_key_ << "." << key_ << ": "
<< "value " << val << " is smaller than the lower bound " << begin;
throw AttrError(os.str());
}
return *this;
}
// This function sets the upper bound of the attribute
TSelf& set_upper_bound(DMLC_ATTRIBUTE_UNUSED const T& end) {
if (this->value_missing_) return *this;
const T& val = *value_;
if (val > end) {
std::ostringstream os;
os << type_key_ << "." << key_ << ": "
<< "value " << val << " is bigger than the upper bound " << end;
throw AttrError(os.str());
}
return *this;
}
// set default when
TSelf& set_default(DMLC_ATTRIBUTE_UNUSED const T& value) {
if (!value_missing_) return *this;
*value_ = value;
value_missing_ = false;
return *this;
}
TSelf& describe(DMLC_ATTRIBUTE_UNUSED const char* str) { return *this; }
};
template <typename FFind>
class AttrInitVisitor {
public:
// Counter of number of matched attributes during visit.
// This is used to decide if there is additional unmatched attributes.
size_t hit_count_{0};
// constructor
AttrInitVisitor(const char* type_key, FFind ffind) : type_key_(type_key), ffind_(ffind) {}
template <typename T>
AttrInitEntry<T> operator()(const char* key, T* value) {
AttrInitEntry<T> opt;
TVMArgValue val;
opt.type_key_ = type_key_;
opt.key_ = key;
opt.value_ = value;
if (ffind_(key, &val)) {
SetValue(value, val);
opt.value_missing_ = false;
++hit_count_;
} else {
opt.value_missing_ = true;
}
return std::move(opt); // Note! force move
}
private:
// the type key
const char* type_key_;
FFind ffind_;
};
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] hanzz2007 commented on issue #6093: [BUG] Exception may be throwed when attrs is constructed
Posted by GitBox <gi...@apache.org>.
hanzz2007 commented on issue #6093:
URL: https://github.com/apache/incubator-tvm/issues/6093#issuecomment-660596761
for example
```C++
struct LoopPartitionConfigNode : public tvm::AttrsNode<LoopPartitionConfigNode> {
bool partition_const_loop;
TVM_DECLARE_ATTRS(LoopPartitionConfigNode, "tir.transform.LoopPartitionConfig") {
//macro expanded: __fvisitor__(xxx, xxx) /*note here, __fvisitor__ return a AttrInitEntry<T> object by value, an copy constructor and destructor will be called */
// .describe().set_default()
TVM_ATTR_FIELD(partition_const_loop).describe("Split constant loop").set_default(false);
}
};
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen closed issue #6093: [BUG] Exception may be throwed when attrs is constructed
Posted by GitBox <gi...@apache.org>.
tqchen closed issue #6093:
URL: https://github.com/apache/incubator-tvm/issues/6093
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on issue #6093: [BUG] Exception may be throwed when attrs is constructed
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #6093:
URL: https://github.com/apache/incubator-tvm/issues/6093#issuecomment-662561068
Sounds good, feel free to send a PR
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on issue #6093: [BUG] Exception may be throwed when attrs is constructed
Posted by GitBox <gi...@apache.org>.
tqchen commented on issue #6093:
URL: https://github.com/apache/incubator-tvm/issues/6093#issuecomment-661486271
Can you elaborate? It is the intended behavior to throw when a value is missing
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] hanzz2007 commented on issue #6093: [BUG] Exception may be throwed when attrs is constructed
Posted by GitBox <gi...@apache.org>.
hanzz2007 commented on issue #6093:
URL: https://github.com/apache/incubator-tvm/issues/6093#issuecomment-661696506
I know it's intended when we want to initialize AttrsNode with arguments, but what if initialize with default value, `if (ffind_(key, &val))` will always return `false`, so `opt.value_missing_` must be `true` in `AttrInitEntry<T> operator()(const char* key, T* value) `,
and when return from `AttrInitEntry<T> operator()`, `opt` will be destructed so exception throwed.
`opt.value_missing_` is set to `false` when `... constant loop").set_default(false);` executed, but it's too late, because `opt` constructed in `AttrInitEntry<T> operator()` has been called destructor function when function finish.
In gcc and llvm, no exception throwed for `opt.~AttrInitEntry()` will not be called because RVO optimization I guess. But it should be a problem in standard way.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org