[GitHub] [tvm] comaniac commented on a change in pull request #9641: [RELAY] [AST] Add virtual_device as a first class field in Relay

[GitBox <gi...@apache.org>]

comaniac commented on a change in pull request #9641:
URL: https://github.com/apache/tvm/pull/9641#discussion_r764397512



##########
File path: include/tvm/relay/expr.h
##########
@@ -240,14 +245,16 @@ class Var : public Expr {
  * \param opt_vid The (optional) vid for the copied var. If none, ret_var->vid = var->vid.
  * \param opt_type_annotation The (optional) type_annotation for the copied var. If none,
  * ret_var->type_annotation = var->type_annotation.
- * \param opt_span The (optional) span for the copied var. If none, ret_var->span = var->span.
- * \return If all properties are null or the same as the property in the input var
- * (i.e., opt_vid is null or opt_vid.value() == var->vid, etc.), then we return var. Otherwise,
- * we return a copy of call with the different fields overwritten. (i.e., if
- * opt_vid.value() != var->vid, then ret_var->vid = opt_.value()).
+ * \param opt_virtual_device The (optional) virtual_device for the copied tuple. If none,
+ * ret_tuple->virtual_device = tuple->virtual_device. \param opt_span The (optional) span for the
+ * copied var. If none, ret_var->span = var->span. \return If all properties are null or the same as
+ * the property in the input var (i.e., opt_vid is null or opt_vid.value() == var->vid, etc.), then
+ * we return var. Otherwise, we return a copy of call with the different fields overwritten. (i.e.,
+ * if opt_vid.value() != var->vid, then ret_var->vid = opt_.value()).

Review comment:
       \param and \return need new lines.

##########
File path: include/tvm/relay/expr.h
##########
@@ -456,15 +465,17 @@ class Let : public Expr {
  * \param opt_var The (optional) var for the copied let. If none, ret_let->op = let->op.
  * \param opt_value The (optional) value for the copied let. If none, ret_let->args = let->args.
  * \param opt_body The (optional) body for the copied let. If none, ret_let->attrs = let->attrs.
- * \param opt_span The (optional) span for the copied let. If none, ret_let->span = let->span.
- * \return If all properties are null or the same as the property in the input let (i.e., opt_var is
- * null or opt_var.value() == let->var, etc.), then we return let. Otherwise, we return a copy of
- * let with the different fields overwritten. (i.e., if opt_var.value() != let->var, then
- * ret_let->var = opt_var.value()).
+ * \param opt_virtual_device The (optional) virtual_device for the copied let. If none,
+ * ret_let->virtual_device = let->virtual_device. \param opt_span The (optional) span for the copied
+ * let. If none, ret_let->span = let->span. \return If all properties are null or the same as the
+ * property in the input let (i.e., opt_var is null or opt_var.value() == let->var, etc.), then we
+ * return let. Otherwise, we return a copy of let with the different fields overwritten. (i.e., if
+ * opt_var.value() != let->var, then ret_let->var = opt_var.value()).

Review comment:
       ditto

##########
File path: include/tvm/relay/expr.h
##########
@@ -784,16 +804,18 @@ class RefWrite : public Expr {
  * ret_ref_write->ref = ref_write->ref.
  * \param opt_value The (optional) value for the copied ref_write. If none,
  * ret_ref_write->value = ref_write->value.
- * \param opt_span
- * The (optional) span for the copied ref_write. If none, ret_ref_write->span = ref_write->span.
- * \return If all properties are null or the same as the property in the input ref_write
- * (i.e., opt_ref is null or opt_ref.value() == ref_write->ref, etc.), then we return ref_write.
- * Otherwise, we return a copy of ref_write with the different fields overwritten.
- * (i.e., if ref_write.value() != ref_write->ref, then
- * ret_ref_write->ref = opt_ref.value()).
+ * \param opt_virtual_device
+ * The (optional) virtual_device for the copied ref_write. If none, ret_ref_write->virtual_device =
+ * ref_write->virtual_device. \param opt_span The (optional) span for the copied ref_write. If none,
+ * ret_ref_write->span = ref_write->span. \return If all properties are null or the same as the
+ * property in the input ref_write (i.e., opt_ref is null or opt_ref.value() == ref_write->ref,
+ * etc.), then we return ref_write. Otherwise, we return a copy of ref_write with the different
+ * fields overwritten. (i.e., if ref_write.value() != ref_write->ref, then ret_ref_write->ref =
+ * opt_ref.value()).

Review comment:
       ditto

##########
File path: include/tvm/ir/expr.h
##########
@@ -165,6 +168,30 @@ class RelayExprNode : public BaseExprNode {
   template <typename TTypeNode>
   inline const TTypeNode* type_as() const;
 
+  /*!
+   * \brief The virtual device (SEScope) for this node (the result of device planning).
+   * For first-order expressions (non functions), this describes where the result of evaluating the
+   * expression should be stored. Note that currently, all composite first-order values (tuples,
+   * references, ADTs) must be stored on the same virtual device. This means that it is not possible
+   * to store two tuple fields on different devices, so we only need one virtual device for these
+   * types.
+   *
+   * For expressions that have the function type, the virtual device describes where the result of
+   * the call to the function or closure is stored (instead of where the function itself is stored).
+   * The SEScope's Target field describes how the body of the function should be compiled.
+   *
+   * \note Unfortunately, the type of virtual_device_ needs to be ObjectRef to avoid a circular
+   * import. We can forward-declare the SEScope type for the getter function, but not for the field
+   *       itself.

Review comment:
       nit: feel that this would be sufficient to explain the reason of using ObjectRef here.
   ```suggestion
      * \note The type of virtual_device_ needs to be ObjectRef to avoid a circular import.
   ```

##########
File path: include/tvm/relay/expr.h
##########
@@ -720,15 +738,17 @@ class RefRead : public Expr {
  * \param ref_read The ref_read to copy.
  * \param opt_ref The (optional) ref for the copied ref_read. If none, ret_ref_read->ref =
  * ref_read->ref.
- * \param opt_span
- * The (optional) span for the copied ref_read. If none, ret_ref_read->span = ref_read->span.
- * \return If all properties are null or the same as the property in the input ref_read
- * (i.e., opt_ref is null or opt_ref.value() == ref_read->ref, etc.), then we return ref_read.
- * Otherwise, we return a copy of ref_read with the different fields overwritten.
- * (i.e., if opt_ref.value() != ref_read->ref, then
- * ret_ref_read->ref = opt_ref.value()).
+ * \param opt_virtual_device
+ * The (optional) virtual_device for the copied ref_read. If none, ret_ref_read->virtual_device =
+ * ref_read->virtual_device. \param opt_span The (optional) span for the copied ref_read. If none,
+ * ret_ref_read->span = ref_read->span. \return If all properties are null or the same as the
+ * property in the input ref_read (i.e., opt_ref is null or opt_ref.value() == ref_read->ref, etc.),
+ * then we return ref_read. Otherwise, we return a copy of ref_read with the different fields
+ * overwritten. (i.e., if opt_ref.value() != ref_read->ref, then ret_ref_read->ref =
+ * opt_ref.value()).

Review comment:
       ditto

##########
File path: include/tvm/relay/expr.h
##########
@@ -362,16 +369,18 @@ class Call : public Expr {
  * 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()).
+ * \param opt_virtual_device The (optional) virtual_device for the copied call. If none,
+ * ret_call->virtual_device = call->virtual_device. \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()).

Review comment:
       ditto

##########
File path: include/tvm/relay/expr.h
##########
@@ -539,17 +550,19 @@ class If : public Expr {
  * ret_if->true_branch = ret_if->false_branch.
  * \param opt_false_branch The (optional) false_branch
  * for the copied if_expr. If none, ret_if->false_branch = if_expr->false_branch.
- * \param opt_span
- * The (optional) span for the copied if_expr. If none, ret_if->span = if_expr->span.
- * \return If all
- * properties are null or the same as the property in the input if_expr (i.e., opt_cond is null or
- * opt_cond.value() == if_expr->cond, etc.), then we return if_expr. Otherwise, we return a copy of
- * if_expr with the different fields overwritten. (i.e., if opt_cond.value() != if_expr->cond, then
- * ret_if->cond = opt_cond.value()).
+ * \param opt_virtual_device The (optional) virtual_device for the copied if_expr. If none,
+ * ret_if->virtual_device = if_expr->virtual_device.
+ * \param opt_span The (optional) span for the copied if_expr. If none,
+ * ret_if->span = if_expr->span.
+ * \return If all properties are null or the same as the property in
+ * the input if_expr (i.e., opt_cond is null or opt_cond.value() == if_expr->cond, etc.), then we
+ * return if_expr. Otherwise, we return a copy of if_expr with the different fields overwritten.
+ * (i.e., if opt_cond.value() != if_expr->cond, then ret_if->cond = opt_cond.value()).

Review comment:
       ditto




-- 
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