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 2021/11/23 21:48:12 UTC
[GitHub] [tvm] electriclilies opened a new pull request #9569: [Relay] WithFields method for Call, Function, Var, TupleGetItem, If, Let, RefCreate, RefRead, RefWrite, Match, and Clause
electriclilies opened a new pull request #9569:
URL: https://github.com/apache/tvm/pull/9569
In this PR, I implement the WithFields method for Call, Function, Var, TupleGetItem, If, Let, RefCreate, RefRead, RefWrite, Match and Clause. It is a continuation of work in #9533, where I implement WithFields for Tuples.
Additionally, I remove the explicit COW logic from the visitors in expr_functor.cc for these classes. In future work, I'll use these methods to replace most explicit reconstructions of these classes in visitors.
@mbs-octoml @mikepapadim PTAL! I would especially appreciate typo checks in the documentation, I did a lot of copy-pasting and probably missed some things there.
cc @tqchen in case you are interested -- I've kept the API consistent with what we discussed for WithFields for tuples
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm merged pull request #9569: [Relay] WithFields method for Call, Function, Var, TupleGetItem, If, Let, RefCreate, RefRead, RefWrite, Match, and Clause
Posted by GitBox <gi...@apache.org>.
manupa-arm merged pull request #9569:
URL: https://github.com/apache/tvm/pull/9569
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies edited a comment on pull request #9569: [Relay] WithFields method for Call, Function, Var, TupleGetItem, If, Let, RefCreate, RefRead, RefWrite, Match, and Clause
Posted by GitBox <gi...@apache.org>.
electriclilies edited a comment on pull request #9569:
URL: https://github.com/apache/tvm/pull/9569#issuecomment-977324782
@mbs-octoml Changed the explicit constructions in the device planner to copy except for one Tuple, since I don't have WithFields for tuples in this PR (waiting on #9533).
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on a change in pull request #9569: [Relay] WithFields method for Call, Function, Var, TupleGetItem, If, Let, RefCreate, RefRead, RefWrite, Match, and Clause
Posted by GitBox <gi...@apache.org>.
electriclilies commented on a change in pull request #9569:
URL: https://github.com/apache/tvm/pull/9569#discussion_r755544735
##########
File path: src/relay/transforms/partial_eval.cc
##########
@@ -615,6 +615,8 @@ class PartialEvaluator : public ExprFunctor<PStatic(const Expr& e, LetList* ll)>
value.push_back(ps);
expr.push_back(ps->dynamic);
}
+ // Note: The partial evaluator seems to do some weird stuff with sharing. Changing Tuple(expr)
+ // to WithFields(op, expr) causes some strange failures.
return HasStatic(MkSTuple(value), ll->Push(Tuple(expr)));
Review comment:
I was seeing some failures in the partial evaluator tests, but also in other places. I'll update the comment to clarify, I don't think I need to add another unittest here though
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on pull request #9569: [Relay] WithFields method for Call, Function, Var, TupleGetItem, If, Let, RefCreate, RefRead, RefWrite, Match, and Clause
Posted by GitBox <gi...@apache.org>.
electriclilies commented on pull request #9569:
URL: https://github.com/apache/tvm/pull/9569#issuecomment-977324782
@mbs-octoml Changed the explicit constructions in the device planner to copy except for one Tuple, since I don't have WithFields for tuples in this PR (waiting on #9533).
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] jroesch commented on a change in pull request #9569: [Relay] WithFields method for Call, Function, Var, TupleGetItem, If, Let, RefCreate, RefRead, RefWrite, Match, and Clause
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #9569:
URL: https://github.com/apache/tvm/pull/9569#discussion_r755595367
##########
File path: src/relay/ir/expr_functor.cc
##########
@@ -512,9 +464,9 @@ class ExprBinder : public MixedModeMutator, PatternMutator {
Pattern VisitPattern(const Pattern& p) final { return PatternMutator::VisitPattern(p); }
- Clause VisitClause(const Clause& c) final {
- Pattern pat = VisitPattern(c->lhs);
- return Clause(pat, VisitExpr(c->rhs));
+ Clause VisitClause(const Clause& clause) final {
+ Pattern lhs = VisitPattern(clause->lhs);
+ return WithFields(std::move(clause), std::move(lhs), VisitExpr(clause->rhs));
Review comment:
@mbs-octoml I don't know if we actually need these moves, for const reference does this matter? I can't actually remember what the spec says about value vs move semantics for const &.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mikepapadim commented on a change in pull request #9569: [Relay] WithFields method for Call, Function, Var, TupleGetItem, If, Let, RefCreate, RefRead, RefWrite, Match, and Clause
Posted by GitBox <gi...@apache.org>.
mikepapadim commented on a change in pull request #9569:
URL: https://github.com/apache/tvm/pull/9569#discussion_r755531291
##########
File path: src/relay/transforms/partial_eval.cc
##########
@@ -615,6 +615,8 @@ class PartialEvaluator : public ExprFunctor<PStatic(const Expr& e, LetList* ll)>
value.push_back(ps);
expr.push_back(ps->dynamic);
}
+ // Note: The partial evaluator seems to do some weird stuff with sharing. Changing Tuple(expr)
+ // to WithFields(op, expr) causes some strange failures.
return HasStatic(MkSTuple(value), ll->Push(Tuple(expr)));
Review comment:
Can we get capture these failures with some unit-test?
##########
File path: src/relay/ir/expr.cc
##########
@@ -184,6 +290,25 @@ TupleGetItem::TupleGetItem(Expr tuple, int index, Span span) {
data_ = std::move(n);
}
+TupleGetItem WithFields(TupleGetItem tuple_get_item, Optional<Expr> opt_tuple,
+ Optional<Integer> opt_index, Optional<Span> opt_span) {
+ Expr tuple = opt_tuple.value_or(tuple_get_item->tuple);
+ int index =
+ opt_index.value_or(tuple_get_item->index)
+ ->value; // Is it OK that this is an Integer instead of int? this lets me use Optional
Review comment:
I think Integer is fine here. Still need this comment?
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbs-octoml commented on a change in pull request #9569: [Relay] WithFields method for Call, Function, Var, TupleGetItem, If, Let, RefCreate, RefRead, RefWrite, Match, and Clause
Posted by GitBox <gi...@apache.org>.
mbs-octoml commented on a change in pull request #9569:
URL: https://github.com/apache/tvm/pull/9569#discussion_r755586614
##########
File path: include/tvm/relay/expr.h
##########
@@ -319,8 +337,31 @@ class Call : public Expr {
Array<Type> type_args = Array<Type>(), Span span = Span());
TVM_DEFINE_OBJECT_REF_METHODS(Call, RelayExpr, CallNode);
+ TVM_DEFINE_OBJECT_REF_COW_METHOD(CallNode);
};
+/*!
+ * \brief Returns the call with given properties. A null property denotes 'no change'.
+ * Returns call if all properties are unchanged. Otherwise, returns a copy with the new fields.
+ * \param call The call to copy.
+ * \param opt_op The (optional) op for the copied call. If none, ret_call->op = call->op.
+ * \param opt_args The (optional) args for the copied call. If none, ret_call->args = call->args.
+ * \param opt_attrs The (optional) attrs for the copied call. If none, ret_call->attrs =
+ * call->attrs.
+ * \param opt_type_args The (optional) type args for the copied call. If none,
+ * ret_call->type_args = call->type_args.
+ * \param opt_span The (optional) span for the copied call. If none, ret_call->span = call->span.
+ * \return If all properties are null or the same as the property in the input call
+ * (i.e., opt_op is null or opt_op.value() == call->op, etc.), then we return call. Otherwise, we
+ * return a copy of call with the different fields overwritten. (i.e., if opt_op.value() !=
+ * call->op, then ret_call->op = opt_op.value()).
+ */
+Call WithFields(Call call, Optional<Expr> opt_op = Optional<Expr>(),
+ Optional<Array<Expr>> opt_args = Optional<Array<Expr>>(),
+ Optional<Attrs> opt_attrs = Optional<Attrs>(),
+ Optional<Array<Type>> opt_type_args = Optional<Array<Type>>(),
+ Optional<Span> opt_span = Optional<Span>(nullptr));
Review comment:
don't need the nullptr arg apparently
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on a change in pull request #9569: [Relay] WithFields method for Call, Function, Var, TupleGetItem, If, Let, RefCreate, RefRead, RefWrite, Match, and Clause
Posted by GitBox <gi...@apache.org>.
electriclilies commented on a change in pull request #9569:
URL: https://github.com/apache/tvm/pull/9569#discussion_r755545150
##########
File path: src/relay/ir/expr.cc
##########
@@ -184,6 +290,25 @@ TupleGetItem::TupleGetItem(Expr tuple, int index, Span span) {
data_ = std::move(n);
}
+TupleGetItem WithFields(TupleGetItem tuple_get_item, Optional<Expr> opt_tuple,
+ Optional<Integer> opt_index, Optional<Span> opt_span) {
+ Expr tuple = opt_tuple.value_or(tuple_get_item->tuple);
+ int index =
+ opt_index.value_or(tuple_get_item->index)
+ ->value; // Is it OK that this is an Integer instead of int? this lets me use Optional
Review comment:
nice catch, thanks!
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on a change in pull request #9569: [Relay] WithFields method for Call, Function, Var, TupleGetItem, If, Let, RefCreate, RefRead, RefWrite, Match, and Clause
Posted by GitBox <gi...@apache.org>.
electriclilies commented on a change in pull request #9569:
URL: https://github.com/apache/tvm/pull/9569#discussion_r755603629
##########
File path: src/relay/ir/expr_functor.cc
##########
@@ -512,9 +464,9 @@ class ExprBinder : public MixedModeMutator, PatternMutator {
Pattern VisitPattern(const Pattern& p) final { return PatternMutator::VisitPattern(p); }
- Clause VisitClause(const Clause& c) final {
- Pattern pat = VisitPattern(c->lhs);
- return Clause(pat, VisitExpr(c->rhs));
+ Clause VisitClause(const Clause& clause) final {
+ Pattern lhs = VisitPattern(clause->lhs);
+ return WithFields(std::move(clause), std::move(lhs), VisitExpr(clause->rhs));
Review comment:
Oh, didn't even notice it was const. I was just std::moving everything that was named that we were not using again
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on pull request #9569: [Relay] WithFields method for Call, Function, Var, TupleGetItem, If, Let, RefCreate, RefRead, RefWrite, Match, and Clause
Posted by GitBox <gi...@apache.org>.
electriclilies commented on pull request #9569:
URL: https://github.com/apache/tvm/pull/9569#issuecomment-978894017
@Mousius or @manupa-arm could you merge? It’s a holiday weekend here and I’d like to get this in before there are any merge conflicts. Thanks!
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org