Remove attributes of relax.print, assert and unique #443
Conversation
| Call alloc_storage( | ||
| mem_alloc_storage, | ||
| {std::move(size), virtual_device_index, StringImm(storage_scope), DataTypeImm(dtype)}, | ||
| Attrs()); | ||
| Call alloc_storage(mem_alloc_storage, {std::move(size), virtual_device_index, | ||
| StringImm(storage_scope), DataTypeImm(dtype)}); |
src/relax/utils.cc
Outdated
| if (const auto* call_node = expr.as<CallNode>()) { | ||
| static const Op& null_value_op_ = Op::Get("relax.null_value"); | ||
| if (call_node->op == null_value_op_) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
I’m wondering about the purpose of this change. Could you elaborate a bit on this?
There was a problem hiding this comment.
Since Call(null_value, {}) is used as None value for argument axis of Unique, it could be regarded as an empty node, and a leaf node
There was a problem hiding this comment.
I think we should be very careful about changing the definition of leaf nodes. Is there a very compelling reason to leave null_value inside the call? If it gets normalized into a binding and, say, the FInferStructInfo for unique needs to check if an attr is null_value, it can use the blockbuilder context to see if the var's definition is a call to null_value (most of the time, it will, and if it's not, you treat it as unknown).
There was a problem hiding this comment.
null_value, though, might be important enough to make an exception for. That is worth thinking about further
There was a problem hiding this comment.
removed Call(null_value, {}) in this pr, since the check will fail. I think it should be an issue in the normalizer. Have tried to use a varbinding, but ran into well-formness issue. So I decided not to use null_value in this pr.
|
note #453, this PR should be part of PRs to send to unity |
|
@yongwww would be great to followup on the state of this pr |
|
@YuchenJin @MasterJH5574 @slyubomirsky I have fixed the comments you left, please take another look. |
| # todo (yongwww): tracked in tlc-pack/relax issue 421 | ||
| # _check(foo, bb.get()["foo"]) |
There was a problem hiding this comment.
Could you mention this test in that issue as well? On the other hand, I’m curious about the reason why the printed IR cannot be parsed back. R.prim_value(xxx) is already acceptable by the parser. As long as we make sure that the Python and FFI interface accept PrimValue, I assume there should be no parsing issue.
There was a problem hiding this comment.
Enabled this test and that one mentioned in #421 in the latest push. Yeah, the issue disappeared after updating the python side to accept PrimValue.
There was a problem hiding this comment.
Great. Thanks for confirming!
|
Let us send it to unity branch |
the remaining effort of #405