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/05/29 19:35:08 UTC
[GitHub] [incubator-tvm] kevinthesun opened a new pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
kevinthesun opened a new pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699
This PR does a major refactor for TensorFlow control flow and tensor array parsing logic:
1. Follow the logic of loop invariant code motion to directly lift any out-of-loop nodes as loop variable. This is a general method better than previous structure hash matching method.
2. Optimize static tensor array shape generation.
This PR is mostly done but depends on https://github.com/apache/incubator-tvm/pull/4312 for end to end SSD testing.
After this PR, TensorFlow frontend should be able to generally support control flow and tensor array, and user can compile TensorFlow object detection model with VM compilation.
@tqchen @zhiics @wweic @yongwww @lixiaoquan
----------------------------------------------------------------
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] kevinthesun commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r438983100
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -2395,29 +2410,40 @@ def _get_abs_layer_name(node):
# 1.x.
_control_flow_nodes = ['Merge', 'Switch', 'NextIteration', 'Exit', 'Enter', 'LoopCond']
-# A map to record tensor array with fixed rank shape
-_static_tensor_array_map = {}
-
-class RewriteSubgraph(ExprMutator):
- """
- A helper class to rewrite expr in while loop function to variable
-
- Parameters
- ----------
- rewrite_map : Dict[expr, expr]
- A dictionay contains a set of expr to var mapping.
- """
- def __init__(self, rewrite_map):
- ExprMutator.__init__(self)
- self.rewrite_map = rewrite_map
-
- def visit(self, expr):
- if expr in self.rewrite_map:
- return self.rewrite_map[expr]
- return super().visit(expr)
+# A map to record tensor array write ops and input ta/tensor indices
+# Value is (index of tensor array, index of written node)
+_tensor_array_write_ops = {
+ "TensorArrayWrite" : (3, 2),
+ "TensorArrayScatter" : (0, 2),
+ "TensorArraySplit" : (0, 1),
+}
-def rewrite_subgraph(expr, rewrites):
- return RewriteSubgraph(rewrites).visit(expr)
+def is_tensor_array_constuctor(tf_node):
+ """Check whether is tensor array constructor node."""
+ is_ta = False
+ ta_start = "TensorArrayV"
+ if tf_node.op.startswith(ta_start):
+ try:
+ int(tf_node.op[len(ta_start)])
Review comment:
Will use ```isnumeric``` instead
----------------------------------------------------------------
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] kevinthesun commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r436123241
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -3194,6 +3191,55 @@ def _convert_operator(self, op_name, inputs, attrs,
raise NotImplementedError("Operator {} not implemented.".format(op_name))
return sym
+ def _licm_construct(self, loop_name, node_name):
+ """Construct a node by considering whether it is
+ loop invariant with the given while loop. If yes, we
+ generate a loop Variable. Otherwise, return regular
+ converted relay expression.
+
+ Parameters
+ ----------
+ loop_name : str
+ TensorFlow while loop name to be checked.
+
+ node_name : str
+ TensorFlow node name.
+
+ Returns
+ -------
+ out : relay.Expr or relay.Var
+ Converted relay expression or loop var.
+ """
+ actual_expr = self._backtrack_construct(node_name)
+ tn = node_name.split(':').split("^")[-1]
+ node_name = tn[0]
+ cloop_name = find_parent_loop_name(node_name, self._while_loop_name_set)
+
+ if loop_name in self._while_loop_name_set and not cloop_name.startswith(loop_name):
Review comment:
I add a fix to do loop var lifting inside while loop construction. This can at least fix the naming issue for those original tf while loop var nodes.
----------------------------------------------------------------
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] kevinthesun commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r432772828
##########
File path: tests/python/frontend/tensorflow/test_forward.py
##########
@@ -94,21 +95,21 @@ def vmobj_to_list(o):
def run_tvm_graph(graph_def, input_data, input_node, num_output=1,
target='llvm', out_names=None, opt_level=3, mode='graph_runtime',
- cuda_layout="NCHW"):
+ layout=None, disabled_pass=None):
Review comment:
We might need to disable fold_scale_axis for ssd testing. https://discuss.tvm.ai/t/relay-pass-do-we-want-to-allow-letnode-in-foldscaleaxis-pass/6420 Will update that when dynamic nms pr is in.
----------------------------------------------------------------
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] kevinthesun commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r433035034
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -788,52 +797,21 @@ def _impl(inputs, attr, params, mod):
def _tensor_array():
def _impl(inputs, attr, params, prelude):
- try:
- from tensorflow.python.framework import tensor_util
- except ImportError as e:
- raise ImportError(
- "Unable to import tensorflow which is required {}".format(e))
-
dtype_str = attr.get('dtype').name
assert not attr["dynamic_size"], "Dynamic size tensor array is " \
"not supported in TVM yet."
- raw_elem_shape = tensor_util.TensorShapeProtoToList(attr['element_shape'])
- elem_shape = []
- for dim in raw_elem_shape:
- if dim < 0:
- elem_shape.append(Any())
- else:
- elem_shape.append(dim)
-
- if elem_shape:
- # Element shape is specified.
- # Directly create static tensor array with given shape.
+ if attr['identical_element_shapes']:
+ assert "shape" in attr, "Shape is required for static tensor array."
Review comment:
shape is set manually here.
----------------------------------------------------------------
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] kevinthesun commented on pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#issuecomment-642969988
@zhiics @yongwww @lixiaoquan Comments addressed. PTAL.
----------------------------------------------------------------
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] kevinthesun commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r432772828
##########
File path: tests/python/frontend/tensorflow/test_forward.py
##########
@@ -94,21 +95,21 @@ def vmobj_to_list(o):
def run_tvm_graph(graph_def, input_data, input_node, num_output=1,
target='llvm', out_names=None, opt_level=3, mode='graph_runtime',
- cuda_layout="NCHW"):
+ layout=None, disabled_pass=None):
Review comment:
We might need to disable fold_scale_axis for ssd testing. https://discuss.tvm.ai/t/relay-pass-do-we-want-to-allow-letnode-in-foldscaleaxis-pass/6420
----------------------------------------------------------------
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] zhiics commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r438517544
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -2444,6 +2470,27 @@ def _in_while_loop(control_flow_node_map, op_name):
return op_name in control_flow_node_map and \
"LoopCond" in control_flow_node_map[op_name]
+class RewriteSubgraph(ExprMutator):
+ """
+ A helper class to rewrite expr in while loop function to variable
Review comment:
Add a blank line
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -2657,86 +2636,78 @@ class Loop:
%6
}
"""
- def __init__(self, mod, loop_name, hash2tfnode,
- node_map, while_loop_name_set):
- self.loop_vars = []
+ def __init__(self, mod, loop_name, lvar2expr):
self.cond = None
self.body = []
self._loop = None
self._mod = mod
self._loop_name = loop_name
- self._hash2tfnode = hash2tfnode
- self._node_map = node_map
- self._while_loop_name_set = while_loop_name_set
+ self._lvar2expr = lvar2expr
+ self.loop_vars = []
+
self.aligned = False
def _while_loop(self):
"""An internal API to create a Relay recursive call for a matched TF
`while_loop` construct.
"""
+ bind_map = {}
wl = tvm.relay.var('while_loop')
-
sb = tvm.relay.scope_builder.ScopeBuilder()
- loop_checker = LoopBound(self._loop_name,
- self._hash2tfnode,
- self._while_loop_name_set)
- for body in self.body:
- loop_checker.visit(body)
-
- loop_vars = []
- bind_map = {}
- loop_var_hash_set = set()
- for var in self.loop_vars:
- loop_var_hash_set.add(s_hash(var))
-
- extra_nodes = []
- for extra_loop_var_name in loop_checker.extra_loop_var_names:
- extra_loop_var_name = extra_loop_var_name.split(':')[0].split("^")[-1]
- extra_node = self._node_map[extra_loop_var_name]
- extra_node = extra_node if isinstance(extra_node, _expr.Tuple) else extra_node[0]
- if s_hash(extra_node) not in loop_var_hash_set:
- self.loop_vars.append(extra_node)
- extra_nodes.append(extra_node)
-
- for i, var in enumerate(self.loop_vars):
- if not isinstance(var, _expr.Var):
- var_chk = _infer_type(var, self._mod)
- var_type = var_chk.checked_type
- else:
- var_type = var.type_annotation
-
- v = tvm.relay.var("loop_var" + str(i), type_annotation=var_type)
- loop_vars.append(v)
- bind_map[var] = v
+ lv_list = []
+ expr_list = []
+ extra_vars = []
+
+ for i, lv in enumerate(self.loop_vars):
+ if self._loop_name not in self._lvar2expr:
+ self._lvar2expr[self._loop_name] = {}
+
+ # Handle the case when loop var is not properly lifted.
+ # This can happen when loop var node name is set accidentally
+ # beginning with loop name.
+ if lv not in self._lvar2expr[self._loop_name]:
+ var_name = "{}_loop_var_{}".format(self._loop_name, i)
+ var_type = _infer_type(lv, self._mod).checked_type
+ loop_var = tvm.relay.var(var_name, type_annotation=var_type)
+ self._lvar2expr[self._loop_name][loop_var] = lv
+ bind_map[lv] = loop_var
+ self.loop_vars[i] = loop_var
+ lv = loop_var
+
+ lv_list.append(lv)
+ expr_list.append(self._lvar2expr[self._loop_name][lv])
+
+ if bind_map:
+ self.cond = rewrite_subgraph(self.cond, bind_map)
+ self.body = [rewrite_subgraph(b, bind_map) for b in self.body]
+ cond = tvm.relay.op.min(self.cond)
- self.cond = rewrite_subgraph(self.cond, bind_map)
- self.body = [rewrite_subgraph(b, bind_map) for b in self.body]
-
- self.body_shape = []
- for body in self.body:
- current_node = body
- shape = _infer_shape(current_node, self._mod)
- while not isinstance(shape, (tuple, list)):
- current_node = current_node.args[-1]
- shape = _infer_shape(current_node, self._mod)
- self.body_shape.append(shape)
+ for lv, exp in self._lvar2expr[self._loop_name].items():
+ if lv not in self.loop_vars:
+ var_checker = VarChecker(lv)
+ used = False
+ for bd in self.body + [cond]:
+ var_checker.visit(bd)
+ if var_checker.used:
+ used = True
Review comment:
can we just move the 3 statements under `if used1` here and remove `used`?
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -1444,15 +1420,50 @@ def _impl(inputs, attr, params, mod):
begin = _get_list_param(params, inputs[1])
end = _get_list_param(params, inputs[2])
stride = _get_list_param(params, inputs[3])
+
begin_mask = int(attr.get('begin_mask', 0))
end_mask = int(attr.get('end_mask', 0))
ellipsis_mask = int(attr.get('ellipsis_mask', 0))
new_axis_mask = int(attr.get('new_axis_mask', 0))
shrink_axis_mask = int(attr.get('shrink_axis_mask', 0))
- data_shape = _infer_shape(inputs[0], mod)
+ in_type = _infer_type(inputs[0], mod)
+ data_shape = get_const_tuple(in_type.checked_type.shape)
data_dim = len(data_shape)
stride_dim = len(stride)
+ # This is a special routine to handle strided_slice after shape_of.
+ # We need this since in some cases we want to do strided_slice on
+ # a partial symbolic shape, such as (1, ?), and get a static shape
+ # (1,). Directly slice on shape_of will result in fully dynamic shape.
+ # TODO(kevinthesun): Can we generalize this process with partial eval?
+ if isinstance(inputs[0], _expr.Call) and "shape_of" in str(inputs[0].op):
+ bg = begin[0]
+ ed = end[0]
+ st = stride[0]
+
+ if ed <= 0 < st:
+ ed += data_shape[0]
+
+ in_shape = _infer_shape(inputs[0].args[0], mod)
+ dtype = in_type.checked_type.dtype
+ out_data = []
+ idx = bg
+ is_constant = True
Review comment:
It looks `is_constant` is not needed.
We can just do `if idx < ed:` with a comment.
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -1444,15 +1420,50 @@ def _impl(inputs, attr, params, mod):
begin = _get_list_param(params, inputs[1])
end = _get_list_param(params, inputs[2])
stride = _get_list_param(params, inputs[3])
+
begin_mask = int(attr.get('begin_mask', 0))
end_mask = int(attr.get('end_mask', 0))
ellipsis_mask = int(attr.get('ellipsis_mask', 0))
new_axis_mask = int(attr.get('new_axis_mask', 0))
shrink_axis_mask = int(attr.get('shrink_axis_mask', 0))
- data_shape = _infer_shape(inputs[0], mod)
+ in_type = _infer_type(inputs[0], mod)
+ data_shape = get_const_tuple(in_type.checked_type.shape)
data_dim = len(data_shape)
stride_dim = len(stride)
+ # This is a special routine to handle strided_slice after shape_of.
+ # We need this since in some cases we want to do strided_slice on
+ # a partial symbolic shape, such as (1, ?), and get a static shape
+ # (1,). Directly slice on shape_of will result in fully dynamic shape.
+ # TODO(kevinthesun): Can we generalize this process with partial eval?
+ if isinstance(inputs[0], _expr.Call) and "shape_of" in str(inputs[0].op):
Review comment:
inputs[0].op == op.get("shape_of") ?
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -2395,29 +2410,40 @@ def _get_abs_layer_name(node):
# 1.x.
_control_flow_nodes = ['Merge', 'Switch', 'NextIteration', 'Exit', 'Enter', 'LoopCond']
-# A map to record tensor array with fixed rank shape
-_static_tensor_array_map = {}
-
-class RewriteSubgraph(ExprMutator):
- """
- A helper class to rewrite expr in while loop function to variable
-
- Parameters
- ----------
- rewrite_map : Dict[expr, expr]
- A dictionay contains a set of expr to var mapping.
- """
- def __init__(self, rewrite_map):
- ExprMutator.__init__(self)
- self.rewrite_map = rewrite_map
-
- def visit(self, expr):
- if expr in self.rewrite_map:
- return self.rewrite_map[expr]
- return super().visit(expr)
+# A map to record tensor array write ops and input ta/tensor indices
+# Value is (index of tensor array, index of written node)
+_tensor_array_write_ops = {
+ "TensorArrayWrite" : (3, 2),
+ "TensorArrayScatter" : (0, 2),
+ "TensorArraySplit" : (0, 1),
+}
-def rewrite_subgraph(expr, rewrites):
- return RewriteSubgraph(rewrites).visit(expr)
+def is_tensor_array_constuctor(tf_node):
+ """Check whether is tensor array constructor node."""
+ is_ta = False
+ ta_start = "TensorArrayV"
+ if tf_node.op.startswith(ta_start):
+ try:
+ int(tf_node.op[len(ta_start)])
Review comment:
the value is not used?
----------------------------------------------------------------
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] kevinthesun commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r435748804
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -3194,6 +3191,55 @@ def _convert_operator(self, op_name, inputs, attrs,
raise NotImplementedError("Operator {} not implemented.".format(op_name))
return sym
+ def _licm_construct(self, loop_name, node_name):
+ """Construct a node by considering whether it is
+ loop invariant with the given while loop. If yes, we
+ generate a loop Variable. Otherwise, return regular
+ converted relay expression.
+
+ Parameters
+ ----------
+ loop_name : str
+ TensorFlow while loop name to be checked.
+
+ node_name : str
+ TensorFlow node name.
+
+ Returns
+ -------
+ out : relay.Expr or relay.Var
+ Converted relay expression or loop var.
+ """
+ actual_expr = self._backtrack_construct(node_name)
+ tn = node_name.split(':').split("^")[-1]
+ node_name = tn[0]
+ cloop_name = find_parent_loop_name(node_name, self._while_loop_name_set)
+
+ if loop_name in self._while_loop_name_set and not cloop_name.startswith(loop_name):
Review comment:
Indeed when user sets tf op name in the way that the graph def node name is in the format of ```loop_name/xxx```, we can't know whether it belongs to a while loop or not. The problem here is tf op name is a part of node name in graph def and there is no ```name``` attribute in node attr. For now I haven't found a better way to do licm node construction. In practice, this case should be rare since while loop name is a complicated hierarchical combination of op and sub-graph names.
----------------------------------------------------------------
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] kevinthesun commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r435748804
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -3194,6 +3191,55 @@ def _convert_operator(self, op_name, inputs, attrs,
raise NotImplementedError("Operator {} not implemented.".format(op_name))
return sym
+ def _licm_construct(self, loop_name, node_name):
+ """Construct a node by considering whether it is
+ loop invariant with the given while loop. If yes, we
+ generate a loop Variable. Otherwise, return regular
+ converted relay expression.
+
+ Parameters
+ ----------
+ loop_name : str
+ TensorFlow while loop name to be checked.
+
+ node_name : str
+ TensorFlow node name.
+
+ Returns
+ -------
+ out : relay.Expr or relay.Var
+ Converted relay expression or loop var.
+ """
+ actual_expr = self._backtrack_construct(node_name)
+ tn = node_name.split(':').split("^")[-1]
+ node_name = tn[0]
+ cloop_name = find_parent_loop_name(node_name, self._while_loop_name_set)
+
+ if loop_name in self._while_loop_name_set and not cloop_name.startswith(loop_name):
Review comment:
Indeed when user sets tf op name in the format of ```loop_name/xxx```, we can't know whether it belongs to a while loop or not. The problem here is tf op name is a part of node name in graph def and there is no ```name``` attribute in node attr. For now I haven't found a better way to do licm node construction. In practice, this case should be rare since while loop name is a complicated hierarchical combination of op and sub-graph names.
----------------------------------------------------------------
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] kevinthesun commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r435748804
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -3194,6 +3191,55 @@ def _convert_operator(self, op_name, inputs, attrs,
raise NotImplementedError("Operator {} not implemented.".format(op_name))
return sym
+ def _licm_construct(self, loop_name, node_name):
+ """Construct a node by considering whether it is
+ loop invariant with the given while loop. If yes, we
+ generate a loop Variable. Otherwise, return regular
+ converted relay expression.
+
+ Parameters
+ ----------
+ loop_name : str
+ TensorFlow while loop name to be checked.
+
+ node_name : str
+ TensorFlow node name.
+
+ Returns
+ -------
+ out : relay.Expr or relay.Var
+ Converted relay expression or loop var.
+ """
+ actual_expr = self._backtrack_construct(node_name)
+ tn = node_name.split(':').split("^")[-1]
+ node_name = tn[0]
+ cloop_name = find_parent_loop_name(node_name, self._while_loop_name_set)
+
+ if loop_name in self._while_loop_name_set and not cloop_name.startswith(loop_name):
Review comment:
Indeed when user sets tf op name in the way that the graph def node name is in the format of ```loop_name/xxx```, we will recognize it to be part of a while loop. The problem here is tf op name is a part of node name in graph def and there is no ```name``` attribute in node attr. For now I haven't found a better way to do licm node construction. In practice, this case should be rare since while loop name is a complicated hierarchical combination of op and sub-graph names.
----------------------------------------------------------------
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] lixiaoquan commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
lixiaoquan commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r433043210
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -955,34 +930,22 @@ def _impl(inputs, attr, params, prelude):
v = tensor_func(inputs[2])
write_func = prelude.get_var('tensor_array_write', dtype_str)
else:
- # For write operation, it is possible to write to a newly create
- # tensor array. We need to check and recreate its input tensor array.
- if input_ta in _static_tensor_array_map and \
- _static_tensor_array_map[input_ta] is None:
- static_tensor_array_ops = StaticTensorArrayOps(prelude,
- dtype_str,
- input_t_shape)
- static_tensor_array_ops.register()
- ta_constructor = prelude.get_var_static('tensor_array',
- dtype_str,
- input_t_shape)
- new_ta = ta_constructor(input_ta.args[0])
- _static_tensor_array_map[input_ta] = new_ta
- input_ta = new_ta
- input_ta_shape = input_t_shape
- else:
- input_ta_rank = len(input_ta_shape)
- assert input_ta_rank == input_rank, "Shape rank mismatch: {} vs {}". \
- format(input_ta_rank, input_rank)
- static_tensor_array_ops = StaticTensorArrayOps(prelude,
- dtype_str,
- input_ta_shape)
- static_tensor_array_ops.register()
+ input_ta_rank = len(input_ta_shape)
+ assert input_ta_rank == input_rank, "Shape rank mismatch: {} vs {}". \
+ format(input_ta_rank, input_rank)
+ static_tensor_array_ops = StaticTensorArrayOps(prelude,
+ dtype_str,
+ input_ta_shape)
+ static_tensor_array_ops.register()
tensor_func = prelude.get_var_static("tensor_constructor",
dtype_str,
input_ta_shape)
- v = tensor_func(inputs[2])
+ actual_shape = _get_more_static_shape(input_t_shape, input_ta_shape)
Review comment:
actual_shape may have tvm.tir.Any in it so it may need be converted before passed to reshape()
----------------------------------------------------------------
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] lixiaoquan commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
lixiaoquan commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r432765056
##########
File path: tests/python/frontend/tensorflow/test_forward.py
##########
@@ -94,21 +95,21 @@ def vmobj_to_list(o):
def run_tvm_graph(graph_def, input_data, input_node, num_output=1,
target='llvm', out_names=None, opt_level=3, mode='graph_runtime',
- cuda_layout="NCHW"):
+ layout=None, disabled_pass=None):
Review comment:
It seems disabled_pass is not specified by any caller
----------------------------------------------------------------
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] lixiaoquan commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
lixiaoquan commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r435668087
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -3194,6 +3191,55 @@ def _convert_operator(self, op_name, inputs, attrs,
raise NotImplementedError("Operator {} not implemented.".format(op_name))
return sym
+ def _licm_construct(self, loop_name, node_name):
+ """Construct a node by considering whether it is
+ loop invariant with the given while loop. If yes, we
+ generate a loop Variable. Otherwise, return regular
+ converted relay expression.
+
+ Parameters
+ ----------
+ loop_name : str
+ TensorFlow while loop name to be checked.
+
+ node_name : str
+ TensorFlow node name.
+
+ Returns
+ -------
+ out : relay.Expr or relay.Var
+ Converted relay expression or loop var.
+ """
+ actual_expr = self._backtrack_construct(node_name)
+ tn = node_name.split(':').split("^")[-1]
+ node_name = tn[0]
+ cloop_name = find_parent_loop_name(node_name, self._while_loop_name_set)
+
+ if loop_name in self._while_loop_name_set and not cloop_name.startswith(loop_name):
Review comment:
This check of cloop_name doesn't work sometimes becuase of op's name. Patch to reproduce
```
diff --git a/tests/python/frontend/tensorflow/test_control_flow.py b/tests/python/frontend/tensorflow/test_control_flow.py
index 90035279b..e2f8ec115 100644
--- a/tests/python/frontend/tensorflow/test_control_flow.py
+++ b/tests/python/frontend/tensorflow/test_control_flow.py
@@ -46,7 +46,7 @@ def check_equal(graph, tf_out, input_map=None):
def test_vanilla_loop():
graph = tf.Graph()
with graph.as_default():
- i = tf.constant(0)
+ i = tf.constant(0, name="while/counter")
def c(i): return tf.less(i, 10)
```
----------------------------------------------------------------
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] yongwww commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
yongwww commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r439130129
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -1177,18 +1137,26 @@ def _impl(inputs, attr, params, mod):
except (IndexError, KeyError, AttributeError):
# Handle symbolic begin
try:
- begin = _infer_value(inputs[1], params).asnumpy().tolist()
+ begin = _infer_value(inputs[1], params, mod).asnumpy().tolist()
except Exception:
begin = inputs[1]
try:
size = _get_list_param(params, inputs[2])
except (IndexError, KeyError, AttributeError):
# Handle symbolic size
try:
- size = _infer_value(inputs[2], params).asnumpy().tolist()
+ size = _infer_value(inputs[2], params, mod).asnumpy().tolist()
except Exception:
size = inputs[2]
- return _op.strided_slice(inputs[0], begin=begin, end=size, slice_mode="size")
+
+ # Align begin and strides for dynamic shape.
+ data_dim = len(_infer_shape(inputs[0], mod))
+ strides = [1] * data_dim
+ if not isinstance(begin, (_expr.Call, _expr.Var)):
+ for _ in range(len(begin), data_dim):
+ begin.append(0)
Review comment:
apply similar append to size? `size.append(-1)`
----------------------------------------------------------------
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] kevinthesun commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r438983100
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -2395,29 +2410,40 @@ def _get_abs_layer_name(node):
# 1.x.
_control_flow_nodes = ['Merge', 'Switch', 'NextIteration', 'Exit', 'Enter', 'LoopCond']
-# A map to record tensor array with fixed rank shape
-_static_tensor_array_map = {}
-
-class RewriteSubgraph(ExprMutator):
- """
- A helper class to rewrite expr in while loop function to variable
-
- Parameters
- ----------
- rewrite_map : Dict[expr, expr]
- A dictionay contains a set of expr to var mapping.
- """
- def __init__(self, rewrite_map):
- ExprMutator.__init__(self)
- self.rewrite_map = rewrite_map
-
- def visit(self, expr):
- if expr in self.rewrite_map:
- return self.rewrite_map[expr]
- return super().visit(expr)
+# A map to record tensor array write ops and input ta/tensor indices
+# Value is (index of tensor array, index of written node)
+_tensor_array_write_ops = {
+ "TensorArrayWrite" : (3, 2),
+ "TensorArrayScatter" : (0, 2),
+ "TensorArraySplit" : (0, 1),
+}
-def rewrite_subgraph(expr, rewrites):
- return RewriteSubgraph(rewrites).visit(expr)
+def is_tensor_array_constuctor(tf_node):
+ """Check whether is tensor array constructor node."""
+ is_ta = False
+ ta_start = "TensorArrayV"
+ if tf_node.op.startswith(ta_start):
+ try:
+ int(tf_node.op[len(ta_start)])
Review comment:
Will use ```isnumeric``` instead
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -2395,29 +2410,40 @@ def _get_abs_layer_name(node):
# 1.x.
_control_flow_nodes = ['Merge', 'Switch', 'NextIteration', 'Exit', 'Enter', 'LoopCond']
-# A map to record tensor array with fixed rank shape
-_static_tensor_array_map = {}
-
-class RewriteSubgraph(ExprMutator):
- """
- A helper class to rewrite expr in while loop function to variable
-
- Parameters
- ----------
- rewrite_map : Dict[expr, expr]
- A dictionay contains a set of expr to var mapping.
- """
- def __init__(self, rewrite_map):
- ExprMutator.__init__(self)
- self.rewrite_map = rewrite_map
-
- def visit(self, expr):
- if expr in self.rewrite_map:
- return self.rewrite_map[expr]
- return super().visit(expr)
+# A map to record tensor array write ops and input ta/tensor indices
+# Value is (index of tensor array, index of written node)
+_tensor_array_write_ops = {
+ "TensorArrayWrite" : (3, 2),
+ "TensorArrayScatter" : (0, 2),
+ "TensorArraySplit" : (0, 1),
+}
-def rewrite_subgraph(expr, rewrites):
- return RewriteSubgraph(rewrites).visit(expr)
+def is_tensor_array_constuctor(tf_node):
+ """Check whether is tensor array constructor node."""
+ is_ta = False
+ ta_start = "TensorArrayV"
+ if tf_node.op.startswith(ta_start):
+ try:
+ int(tf_node.op[len(ta_start)])
Review comment:
Will use ```isnumeric``` instead
----------------------------------------------------------------
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] zhiics commented on pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
zhiics commented on pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#issuecomment-643080759
Thanks @kevinthesun @yongwww @lixiaoquan
----------------------------------------------------------------
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] yongwww commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
yongwww commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r434980636
##########
File path: tests/python/frontend/tensorflow/test_forward.py
##########
@@ -2238,6 +2258,40 @@ def test_forward_resnetv2():
tvm.testing.assert_allclose(np.squeeze(tvm_output[0]), np.squeeze(tf_output[0]),
rtol=1e-5, atol=1e-5)
+#######################################################################
+# SSD
+# ---
+
+
+def test_forward_ssd():
+ '''test resnet model'''
Review comment:
```suggestion
'''test SSD with backbone MobileNet V1'''
```
##########
File path: tests/python/frontend/tensorflow/test_forward.py
##########
@@ -2238,6 +2258,40 @@ def test_forward_resnetv2():
tvm.testing.assert_allclose(np.squeeze(tvm_output[0]), np.squeeze(tf_output[0]),
rtol=1e-5, atol=1e-5)
+#######################################################################
+# SSD
+# ---
+
+
+def test_forward_ssd():
+ '''test resnet model'''
+ if is_gpu_available():
+ with tf.Graph().as_default():
+ graph_def = tf_testing.get_workload(
+ "object_detection/ssd_mobilenet_v1_ppn_shared_"
+ "box_predictor_300x300_coco14_sync_2018_07_03.pb")
+ # Call the utility to import the graph definition into default graph.
+ graph_def = tf_testing.ProcessGraphDefParam(graph_def)
+
+ data = np.random.uniform(0.0, 255.0, size=(1, 512, 512, 3)).astype('float32')
+ in_node = "image_tensor"
+ out_node = ['detection_boxes', "detection_scores", "detection_classes"]
+
+ with tf.Session() as sess:
+ tf_output = run_tf_graph(
+ sess, data, '{}:0'.format(in_node), ["{}:0".format(oname) for oname in out_node])
+ # TODO(kevinthesun): enable gpu test when VM heterogeneous execution is ready.
+ for device in ["llvm"]:
+ ctx = tvm.context(device, 0)
+ if not ctx.exist:
+ print("Skip because %s is not enabled" % device)
+ continue
+ tvm_output = run_tvm_graph(graph_def, data, in_node, len(out_node),
+ target=device, layout="NCHW", mode="vm")
+ for i in range(len(out_node)):
+ tvm.testing.assert_allclose(tvm_output[i], tf_output[i],
+ rtol=1e-3, atol=1e-3)
Review comment:
```suggestion
def _test_forward_ssd(model_url, in_node, out_nodes, in_shape):
graph_def = tf_testing.get_workload(model_url)
# Call the utility to import the graph definition into default graph.
graph_def = tf_testing.ProcessGraphDefParam(graph_def)
data = np.random.uniform(0.0, 255.0, size=in_shape).astype('float32')
with tf.Session() as sess:
tf_output = run_tf_graph(
sess, data, '{}:0'.format(in_node), ["{}:0".format(oname) for oname in out_nodes])
# TODO(kevinthesun): enable gpu test when VM heterogeneous execution is ready.
for device in ["llvm"]:
ctx = tvm.context(device, 0)
if not ctx.exist:
print("Skip because %s is not enabled" % device)
continue
tvm_output = run_tvm_graph(graph_def, data, in_node, len(out_nodes),
target=device, layout="NCHW", mode="vm")
for i in range(len(out_nodes)):
tvm.testing.assert_allclose(tvm_output[i], tf_output[i],
rtol=1e-3, atol=1e-3)
_test_forward_ssd("object_detection/ssd_mobilenet_v1_ppn_shared_"
"box_predictor_300x300_coco14_sync_2018_07_03.pb",
"image_tensor",
['detection_boxes', "detection_scores", "detection_classes"],
(1, 512, 512, 3))
```
----------------------------------------------------------------
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] kevinthesun commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r439144860
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -1177,18 +1137,26 @@ def _impl(inputs, attr, params, mod):
except (IndexError, KeyError, AttributeError):
# Handle symbolic begin
try:
- begin = _infer_value(inputs[1], params).asnumpy().tolist()
+ begin = _infer_value(inputs[1], params, mod).asnumpy().tolist()
except Exception:
begin = inputs[1]
try:
size = _get_list_param(params, inputs[2])
except (IndexError, KeyError, AttributeError):
# Handle symbolic size
try:
- size = _infer_value(inputs[2], params).asnumpy().tolist()
+ size = _infer_value(inputs[2], params, mod).asnumpy().tolist()
except Exception:
size = inputs[2]
- return _op.strided_slice(inputs[0], begin=begin, end=size, slice_mode="size")
+
+ # Align begin and strides for dynamic shape.
+ data_dim = len(_infer_shape(inputs[0], mod))
+ strides = [1] * data_dim
+ if not isinstance(begin, (_expr.Call, _expr.Var)):
+ for _ in range(len(begin), data_dim):
+ begin.append(0)
Review comment:
Added.
----------------------------------------------------------------
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] kevinthesun commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r435748804
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -3194,6 +3191,55 @@ def _convert_operator(self, op_name, inputs, attrs,
raise NotImplementedError("Operator {} not implemented.".format(op_name))
return sym
+ def _licm_construct(self, loop_name, node_name):
+ """Construct a node by considering whether it is
+ loop invariant with the given while loop. If yes, we
+ generate a loop Variable. Otherwise, return regular
+ converted relay expression.
+
+ Parameters
+ ----------
+ loop_name : str
+ TensorFlow while loop name to be checked.
+
+ node_name : str
+ TensorFlow node name.
+
+ Returns
+ -------
+ out : relay.Expr or relay.Var
+ Converted relay expression or loop var.
+ """
+ actual_expr = self._backtrack_construct(node_name)
+ tn = node_name.split(':').split("^")[-1]
+ node_name = tn[0]
+ cloop_name = find_parent_loop_name(node_name, self._while_loop_name_set)
+
+ if loop_name in self._while_loop_name_set and not cloop_name.startswith(loop_name):
Review comment:
Indeed when user sets tf op name in the way that the graph def node name is in the format of ```loop_name/xxx```, we can incorrectly recognize it to be part of a while loop. The problem here is tf op name is a part of node name in graph def and there is no ```name``` attribute in node attr. For now I haven't found a better way to do licm node construction. In practice, this case should be rare since while loop name is a complicated hierarchical combination of op and sub-graph names.
----------------------------------------------------------------
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] lixiaoquan commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
lixiaoquan commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r433034145
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -788,52 +797,21 @@ def _impl(inputs, attr, params, mod):
def _tensor_array():
def _impl(inputs, attr, params, prelude):
- try:
- from tensorflow.python.framework import tensor_util
- except ImportError as e:
- raise ImportError(
- "Unable to import tensorflow which is required {}".format(e))
-
dtype_str = attr.get('dtype').name
assert not attr["dynamic_size"], "Dynamic size tensor array is " \
"not supported in TVM yet."
- raw_elem_shape = tensor_util.TensorShapeProtoToList(attr['element_shape'])
- elem_shape = []
- for dim in raw_elem_shape:
- if dim < 0:
- elem_shape.append(Any())
- else:
- elem_shape.append(dim)
-
- if elem_shape:
- # Element shape is specified.
- # Directly create static tensor array with given shape.
+ if attr['identical_element_shapes']:
+ assert "shape" in attr, "Shape is required for static tensor array."
Review comment:
Should it be element_shape?
https://github.com/tensorflow/tensorflow/blob/a34091e538540aad57a7a941575538944f38db24/tensorflow/core/kernels/tensor_array_ops.cc#L165
----------------------------------------------------------------
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] zhiics commented on pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
zhiics commented on pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#issuecomment-642977258
@lixiaoquan @yongwww please take another look when you have time.
----------------------------------------------------------------
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] zhiics merged pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
zhiics merged pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699
----------------------------------------------------------------
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] lixiaoquan commented on a change in pull request #5699: [Frontend][TensorFlow] Improve Control Flow and TensorArray
Posted by GitBox <gi...@apache.org>.
lixiaoquan commented on a change in pull request #5699:
URL: https://github.com/apache/incubator-tvm/pull/5699#discussion_r433034145
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -788,52 +797,21 @@ def _impl(inputs, attr, params, mod):
def _tensor_array():
def _impl(inputs, attr, params, prelude):
- try:
- from tensorflow.python.framework import tensor_util
- except ImportError as e:
- raise ImportError(
- "Unable to import tensorflow which is required {}".format(e))
-
dtype_str = attr.get('dtype').name
assert not attr["dynamic_size"], "Dynamic size tensor array is " \
"not supported in TVM yet."
- raw_elem_shape = tensor_util.TensorShapeProtoToList(attr['element_shape'])
- elem_shape = []
- for dim in raw_elem_shape:
- if dim < 0:
- elem_shape.append(Any())
- else:
- elem_shape.append(dim)
-
- if elem_shape:
- # Element shape is specified.
- # Directly create static tensor array with given shape.
+ if attr['identical_element_shapes']:
+ assert "shape" in attr, "Shape is required for static tensor array."
Review comment:
Should it be element_shape? I run into some pb files which don't have attr['shape'] and they can be imported by old code
https://github.com/tensorflow/tensorflow/blob/a34091e538540aad57a7a941575538944f38db24/tensorflow/core/kernels/tensor_array_ops.cc#L165
----------------------------------------------------------------
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