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/10/06 11:55:33 UTC
[GitHub] [tvm] masahi opened a new pull request #9207: [TOPI] Fix compiing batch_matmul and dense when two args are the same tensor
masahi opened a new pull request #9207:
URL: https://github.com/apache/tvm/pull/9207
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.
--
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] masahi commented on a change in pull request #9207: [TOPI] Fix compiing batch_matmul and dense when two args are the same tensor
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #9207:
URL: https://github.com/apache/tvm/pull/9207#discussion_r723650378
##########
File path: python/tvm/relay/op/strategy/generic.py
##########
@@ -715,8 +715,14 @@ def dilation2d_strategy(attrs, inputs, out_type, target):
return strategy
+def maybe_copy_tensor_b(tensor_a, tensor_b):
Review comment:
Yeah I didn't put much thought into the name, I like your suggestion. I'll also add a comment explaining why this is necessary.
##########
File path: python/tvm/relay/op/strategy/generic.py
##########
@@ -715,8 +715,14 @@ def dilation2d_strategy(attrs, inputs, out_type, target):
return strategy
+def maybe_copy_tensor_b(tensor_a, tensor_b):
+ if tensor_a == tensor_b:
+ return te.compute(tensor_a.shape, lambda *ind: tensor_a[ind], tag="tensor_b_copy")
+ return tensor_b
+
+
# matmul
-def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False):
+def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False, need_tensor_b_copy=True):
Review comment:
This is flag is introduced solely for external libs like cublas, which doesn't need copying if two args are the same. I explicitly set `need_tensor_b_copy=False` for cublas strategy only, otherwise we do the copy by default.
##########
File path: python/tvm/topi/cuda/batch_matmul.py
##########
@@ -93,6 +93,8 @@ def schedule_batch_matmul(cfg, outs):
def _schedule(cfg, op):
C = op.output(0)
A, B = s[C].op.input_tensors
+ if B.op.tag == "tensor_b_copy":
+ s[B].compute_inline()
Review comment:
Thanks, that seems to work. I'll make this change. I also prefer your approach since relying on implicit agreement on the tag name is opaque and not robust.
##########
File path: python/tvm/relay/op/strategy/generic.py
##########
@@ -715,8 +715,14 @@ def dilation2d_strategy(attrs, inputs, out_type, target):
return strategy
+def maybe_copy_tensor_b(tensor_a, tensor_b):
+ if tensor_a == tensor_b:
+ return te.compute(tensor_a.shape, lambda *ind: tensor_a[ind], tag="tensor_b_copy")
+ return tensor_b
+
+
# matmul
-def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False):
+def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False, need_tensor_b_copy=True):
Review comment:
Interesting, if we don't have to worry about external libs, I'd choose a different for sure. Actually, we probably don't need this flag at all, since we can always copy anyway.
##########
File path: python/tvm/relay/op/strategy/generic.py
##########
@@ -715,8 +715,14 @@ def dilation2d_strategy(attrs, inputs, out_type, target):
return strategy
+def maybe_copy_tensor_b(tensor_a, tensor_b):
+ if tensor_a == tensor_b:
+ return te.compute(tensor_a.shape, lambda *ind: tensor_a[ind], tag="tensor_b_copy")
+ return tensor_b
+
+
# matmul
-def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False):
+def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False, need_tensor_b_copy=True):
Review comment:
Interesting, if we don't have to worry about external libs, I'd choose a different for sure. Actually, we probably don't need this flag at all, since we can always (and must) copy anyway if two args are the same.
--
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] comaniac commented on a change in pull request #9207: [TOPI] Fix compiing batch_matmul and dense when two args are the same tensor
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #9207:
URL: https://github.com/apache/tvm/pull/9207#discussion_r723643217
##########
File path: python/tvm/relay/op/strategy/generic.py
##########
@@ -715,8 +715,14 @@ def dilation2d_strategy(attrs, inputs, out_type, target):
return strategy
+def maybe_copy_tensor_b(tensor_a, tensor_b):
+ if tensor_a == tensor_b:
+ return te.compute(tensor_a.shape, lambda *ind: tensor_a[ind], tag="tensor_b_copy")
+ return tensor_b
+
+
# matmul
-def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False):
+def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False, need_tensor_b_copy=True):
Review comment:
`need_tensor_b_copy` seems a bit confusing. If tensor a and tensor b are identical, then we could say either copy tensor a or copy tensor b. Maybe `copy_input_if_the_same` or `copy_input_if_identical`, or `copy_identical_input`?
##########
File path: python/tvm/relay/op/strategy/generic.py
##########
@@ -715,8 +715,14 @@ def dilation2d_strategy(attrs, inputs, out_type, target):
return strategy
+def maybe_copy_tensor_b(tensor_a, tensor_b):
Review comment:
I'm wondering if we could have a better name (with docstring) for this utility. Something like `copy_if_identical`?
##########
File path: python/tvm/topi/cuda/batch_matmul.py
##########
@@ -93,6 +93,8 @@ def schedule_batch_matmul(cfg, outs):
def _schedule(cfg, op):
C = op.output(0)
A, B = s[C].op.input_tensors
+ if B.op.tag == "tensor_b_copy":
+ s[B].compute_inline()
Review comment:
Could we use like `B.op.input_tensors[0] == A` to judge whether it is a copy instead of leveraging the tag? IMHO, it's safe to assume that B is a copy of A if we can reach to A from B in batch_matmul/dense compute.
--
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] masahi commented on a change in pull request #9207: [TOPI] Fix compiing batch_matmul and dense when two args are the same tensor
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #9207:
URL: https://github.com/apache/tvm/pull/9207#discussion_r723659378
##########
File path: python/tvm/relay/op/strategy/generic.py
##########
@@ -715,8 +715,14 @@ def dilation2d_strategy(attrs, inputs, out_type, target):
return strategy
+def maybe_copy_tensor_b(tensor_a, tensor_b):
+ if tensor_a == tensor_b:
+ return te.compute(tensor_a.shape, lambda *ind: tensor_a[ind], tag="tensor_b_copy")
+ return tensor_b
+
+
# matmul
-def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False):
+def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False, need_tensor_b_copy=True):
Review comment:
Interesting, if we don't have to worry about external libs, I'd choose a different for sure. Actually, we probably don't need this flag at all, since we can always (and must) copy anyway if two args are the same.
--
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] comaniac commented on a change in pull request #9207: [TOPI] Fix compiing batch_matmul and dense when two args are the same tensor
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #9207:
URL: https://github.com/apache/tvm/pull/9207#discussion_r723643217
##########
File path: python/tvm/relay/op/strategy/generic.py
##########
@@ -715,8 +715,14 @@ def dilation2d_strategy(attrs, inputs, out_type, target):
return strategy
+def maybe_copy_tensor_b(tensor_a, tensor_b):
+ if tensor_a == tensor_b:
+ return te.compute(tensor_a.shape, lambda *ind: tensor_a[ind], tag="tensor_b_copy")
+ return tensor_b
+
+
# matmul
-def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False):
+def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False, need_tensor_b_copy=True):
Review comment:
`need_tensor_b_copy` seems a bit confusing. If tensor a and tensor b are identical, then we could say either copy tensor a or copy tensor b. Maybe `copy_input_if_the_same` or `copy_input_if_identical`, or `copy_identical_input`?
##########
File path: python/tvm/relay/op/strategy/generic.py
##########
@@ -715,8 +715,14 @@ def dilation2d_strategy(attrs, inputs, out_type, target):
return strategy
+def maybe_copy_tensor_b(tensor_a, tensor_b):
Review comment:
I'm wondering if we could have a better name (with docstring) for this utility. Something like `copy_if_identical`?
##########
File path: python/tvm/topi/cuda/batch_matmul.py
##########
@@ -93,6 +93,8 @@ def schedule_batch_matmul(cfg, outs):
def _schedule(cfg, op):
C = op.output(0)
A, B = s[C].op.input_tensors
+ if B.op.tag == "tensor_b_copy":
+ s[B].compute_inline()
Review comment:
Could we use like `B.op.input_tensors[0] == A` to judge whether it is a copy instead of leveraging the tag? IMHO, it's safe to assume that B is a copy of A if we can reach to A from B in batch_matmul/dense compute.
##########
File path: python/tvm/relay/op/strategy/generic.py
##########
@@ -715,8 +715,14 @@ def dilation2d_strategy(attrs, inputs, out_type, target):
return strategy
+def maybe_copy_tensor_b(tensor_a, tensor_b):
+ if tensor_a == tensor_b:
+ return te.compute(tensor_a.shape, lambda *ind: tensor_a[ind], tag="tensor_b_copy")
+ return tensor_b
+
+
# matmul
-def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False):
+def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False, need_tensor_b_copy=True):
Review comment:
Yeah I understand this semantic (btw, IIUC, external libs won't use the TE compute anyways, so does it matter to copy it in the TE compute even for the external libs?)
My comment here is mainly for the naming (if I misunderstood something and we still need this flag for external libs). What I meant was whether we should use a more general naming for this flag.
--
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] masahi commented on a change in pull request #9207: [TOPI] Fix compiing batch_matmul and dense when two args are the same tensor
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #9207:
URL: https://github.com/apache/tvm/pull/9207#discussion_r723650378
##########
File path: python/tvm/relay/op/strategy/generic.py
##########
@@ -715,8 +715,14 @@ def dilation2d_strategy(attrs, inputs, out_type, target):
return strategy
+def maybe_copy_tensor_b(tensor_a, tensor_b):
Review comment:
Yeah I didn't put much thought into the name, I like your suggestion. I'll also add a comment explaining why this is necessary.
--
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] masahi commented on pull request #9207: [TOPI] Fix compiing batch_matmul and dense when two args are the same tensor
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #9207:
URL: https://github.com/apache/tvm/pull/9207#issuecomment-938495450
thanks @comaniac
--
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] masahi merged pull request #9207: [TOPI] Fix compiing batch_matmul and dense when two args are the same tensor
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #9207:
URL: https://github.com/apache/tvm/pull/9207
--
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] masahi commented on a change in pull request #9207: [TOPI] Fix compiing batch_matmul and dense when two args are the same tensor
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #9207:
URL: https://github.com/apache/tvm/pull/9207#discussion_r723659378
##########
File path: python/tvm/relay/op/strategy/generic.py
##########
@@ -715,8 +715,14 @@ def dilation2d_strategy(attrs, inputs, out_type, target):
return strategy
+def maybe_copy_tensor_b(tensor_a, tensor_b):
+ if tensor_a == tensor_b:
+ return te.compute(tensor_a.shape, lambda *ind: tensor_a[ind], tag="tensor_b_copy")
+ return tensor_b
+
+
# matmul
-def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False):
+def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False, need_tensor_b_copy=True):
Review comment:
Interesting, if we don't have to worry about external libs, I'd choose a different for sure. Actually, we probably don't need this flag at all, since we can always copy anyway.
--
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] masahi commented on a change in pull request #9207: [TOPI] Fix compiing batch_matmul and dense when two args are the same tensor
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #9207:
URL: https://github.com/apache/tvm/pull/9207#discussion_r723655456
##########
File path: python/tvm/topi/cuda/batch_matmul.py
##########
@@ -93,6 +93,8 @@ def schedule_batch_matmul(cfg, outs):
def _schedule(cfg, op):
C = op.output(0)
A, B = s[C].op.input_tensors
+ if B.op.tag == "tensor_b_copy":
+ s[B].compute_inline()
Review comment:
Thanks, that seems to work. I'll make this change. I also prefer your approach since relying on implicit agreement on the tag name is opaque and not robust.
--
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] masahi commented on a change in pull request #9207: [TOPI] Fix compiing batch_matmul and dense when two args are the same tensor
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #9207:
URL: https://github.com/apache/tvm/pull/9207#discussion_r723651665
##########
File path: python/tvm/relay/op/strategy/generic.py
##########
@@ -715,8 +715,14 @@ def dilation2d_strategy(attrs, inputs, out_type, target):
return strategy
+def maybe_copy_tensor_b(tensor_a, tensor_b):
+ if tensor_a == tensor_b:
+ return te.compute(tensor_a.shape, lambda *ind: tensor_a[ind], tag="tensor_b_copy")
+ return tensor_b
+
+
# matmul
-def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False):
+def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False, need_tensor_b_copy=True):
Review comment:
This is flag is introduced solely for external libs like cublas, which doesn't need copying if two args are the same. I explicitly set `need_tensor_b_copy=False` for cublas strategy only, otherwise we do the copy by default.
--
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] comaniac commented on a change in pull request #9207: [TOPI] Fix compiing batch_matmul and dense when two args are the same tensor
Posted by GitBox <gi...@apache.org>.
comaniac commented on a change in pull request #9207:
URL: https://github.com/apache/tvm/pull/9207#discussion_r723656913
##########
File path: python/tvm/relay/op/strategy/generic.py
##########
@@ -715,8 +715,14 @@ def dilation2d_strategy(attrs, inputs, out_type, target):
return strategy
+def maybe_copy_tensor_b(tensor_a, tensor_b):
+ if tensor_a == tensor_b:
+ return te.compute(tensor_a.shape, lambda *ind: tensor_a[ind], tag="tensor_b_copy")
+ return tensor_b
+
+
# matmul
-def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False):
+def wrap_compute_matmul(topi_compute, need_auto_scheduler_layout=False, need_tensor_b_copy=True):
Review comment:
Yeah I understand this semantic (btw, IIUC, external libs won't use the TE compute anyways, so does it matter to copy it in the TE compute even for the external libs?)
My comment here is mainly for the naming (if I misunderstood something and we still need this flag for external libs). What I meant was whether we should use a more general naming for this flag.
--
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