[Relay] Reference #2489
[Relay] Reference #2489
Conversation
|
@ZihengJiang @junrushao1994 @zhiics can you guys review this? Thx! |
| Type VisitExpr_(const RefReadNode* op) final { | ||
| Type it = IncompleteTypeNode::make(TypeVarNode::Kind::kType); | ||
| this->Unify(GetType(op->ref), RefTypeNode::make(it), op->span); | ||
| return it; |
There was a problem hiding this comment.
Why unify with a fresh incomplete type? You could just return GetType(op->ref)
There was a problem hiding this comment.
this propagate the constraint backward (it unify op->ref with a reftype
There was a problem hiding this comment.
Ah, I misread the second line. Thanks for clarifying
|
Unit tests for new alpha equality and type inference cases would be helpful for pinpointing bugs/regression. |
src/relay/ir/expr.cc
Outdated
|
|
||
| TVM_REGISTER_API("relay._expr.TempExprRealize") | ||
| TVM_REGISTER_API("relay._make.RefNew").set_body([](TVMArgs args, TVMRetValue* ret) { |
There was a problem hiding this comment.
another line for .set_body
| class RefWrite(Expr): | ||
| """ | ||
| Update the value inside the reference. | ||
| The whole expression will evaluate to an empty tuple. |
|
I only have some minor comments about the PR. But I do have one concern about adding AST nodes/types because it would affect most relay passes. I see that some passes are modified correspondingly, but how about others such as memory planning and forward rewriting, etc? |
include/tvm/relay/expr.h
Outdated
|
|
||
| RELAY_DEFINE_NODE_REF(RefRead, RefReadNode, Expr); | ||
|
|
||
| /*! \brief Set value of Reference. The whole expression evaluate to an Empty Tuple. */ |
There was a problem hiding this comment.
| /*! \brief Set value of Reference. The whole expression evaluate to an Empty Tuple. */ | |
| /*! \brief Set value of Reference. The whole expression evaluates to an Empty Tuple. */ |
include/tvm/relay/expr.h
Outdated
| TVM_DECLARE_NODE_TYPE_INFO(TupleGetItemNode, ExprNode); | ||
| }; | ||
|
|
||
| RELAY_DEFINE_NODE_REF(TupleGetItem, TupleGetItemNode, Expr); | ||
|
|
||
| /*! \brief Create a new Reference out of initial value. */ | ||
| class RefNew; |
| Value r = Eval(op->ref); | ||
| if (const RefValueNode* rv = r.as<RefValueNode>()) { | ||
| rv->value = Eval(op->value); | ||
| return TupleValueNode::make({}); |
There was a problem hiding this comment.
Could you please explain why empty tuple? Is it because the value is already updated?
There was a problem hiding this comment.
yes. there is nothing needed to be return, so an empty tuple will do the job.
There was a problem hiding this comment.
That is also the convention in most ML-like languages.
|
@zhiics how do you do memory planning on control flow? i imagine planning for reference will be similar. |
* move fix test fix lint fix test add more code fix lint better type infer ability * fix build * address comment
* move fix test fix lint fix test add more code fix lint better type infer ability * fix build * address comment
* move fix test fix lint fix test add more code fix lint better type infer ability * fix build * address comment
See #2483