You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by "yongwww (via GitHub)" <gi...@apache.org> on 2023/03/03 01:23:40 UTC
[GitHub] [tvm] yongwww opened a new pull request, #14183: [Unity] Introduce call_dps_packed
yongwww opened a new pull request, #14183:
URL: https://github.com/apache/tvm/pull/14183
Introduce call_dps_packed as https://github.com/tlc-pack/relax/issues/430 mentioned.
--
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] tvm-bot commented on pull request #14183: [Unity] Introduce call_dps_packed
Posted by "tvm-bot (via GitHub)" <gi...@apache.org>.
tvm-bot commented on PR #14183:
URL: https://github.com/apache/tvm/pull/14183#issuecomment-1452800480
<!---bot-comment-->
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @-ing them in a comment.
<sub>Generated by [tvm-bot](https://github.com/apache/tvm/blob/main/ci/README.md#github-actions)</sub>
--
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] slyubomirsky commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "slyubomirsky (via GitHub)" <gi...@apache.org>.
slyubomirsky commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1130144884
##########
src/relax/op/op.cc:
##########
@@ -65,6 +65,9 @@ StructInfo InferStructInfoCallTIR(const Call& call, const BlockBuilder& ctx) {
ctx->ReportFatal(Diagnostic::Error(call)
<< "sinfo_args should have exact 1 output struct info.");
}
+ CHECK(call->args[0]->IsInstance<GlobalVarNode>())
+ << "call_tir expects the first argument as a GlobalVar referring tir PrimFunc. "
Review Comment:
```suggestion
<< "call_tir expects the first argument to be a GlobalVar referring to a TIR PrimFunc. "
```
--
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] yongwww commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "yongwww (via GitHub)" <gi...@apache.org>.
yongwww commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1128714281
##########
tests/python/relax/test_ast_printer.py:
##########
@@ -432,7 +432,7 @@ def test_call_tir():
@R.function
def foo(x: R.Tensor(("m", "n"), "float32")):
m, n = T.var("int64"), T.var("int64")
- gv0 = R.call_tir("test.op.identity", (x,), R.Tensor((m, n), dtype="float32"))
+ gv0 = R.call_dps_packed("test.op.identity", (x,), R.Tensor((m, n), dtype="float32"))
Review Comment:
Have updated the test case to use call_tir 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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] slyubomirsky commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "slyubomirsky (via GitHub)" <gi...@apache.org>.
slyubomirsky commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1130165937
##########
python/tvm/relax/op/base.py:
##########
@@ -51,12 +51,12 @@ def call_tir(
tir_vars: Optional[Union[ShapeExpr, Tuple[PrimExpr], List[PrimExpr]]] = None,
) -> Call:
"""
- Call a destination-passing-style function and return the output.
+ Call a tir.prim_func function and return the output.
Parameters
----------
func : Union[str, Expr]
Review Comment:
I think the signature in Python should reflect that we expect the argument to be a global var if that's how we're going to implement it.
--
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] slyubomirsky commented on pull request #14183: [Unity] Introduce call_dps_packed
Posted by "slyubomirsky (via GitHub)" <gi...@apache.org>.
slyubomirsky commented on PR #14183:
URL: https://github.com/apache/tvm/pull/14183#issuecomment-1454129165
> > Do we have any way to enforce that the function passed to `call_tir` is a `PrimFunc`? I believe making `call_tir` more specific was discussed before. (Should we still permit passing a `PrimFunc` to `call_dps_packed`?)
>
> We can enforce it in `MakeCallTIR` and `MakeCallDPSPacked`. For now I don't see a reason to allow `call_dps_packed` to call `Primfunc`. cc @tqchen
It would have to be in the implementation as well, since you can do something like assign a TIR function to a local var and pass that local var elsewhere. In a higher-order function, we wouldn't be able to detect that at compile time unless we make TIR functions part of the type system (though that could be an option).
--
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] yongwww commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "yongwww (via GitHub)" <gi...@apache.org>.
yongwww commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1131606681
##########
src/script/printer/relax/call.cc:
##########
@@ -134,7 +138,7 @@ Optional<ExprDoc> PrintCallTIR(const relax::Call& n, const ObjectPath& n_p, cons
TVM_STATIC_IR_FUNCTOR(IRDocsifier, vtable)
.set_dispatch<relax::Call>( //
"", [](relax::Call n, ObjectPath n_p, IRDocsifier d) -> Doc {
- // Special case: call_tir
+ // Special case: call_tir, call_dps_packed
if (Optional<ExprDoc> doc = PrintCallTIR(n, n_p, d)) {
Review Comment:
changed it to `PrintCallTIRDPSPacked`
--
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] psrivas2 commented on pull request #14183: [Unity] Introduce call_dps_packed
Posted by "psrivas2 (via GitHub)" <gi...@apache.org>.
psrivas2 commented on PR #14183:
URL: https://github.com/apache/tvm/pull/14183#issuecomment-1453646491
> Do we have any way to enforce that the function passed to `call_tir` is a `PrimFunc`? I believe making `call_tir` more specific was discussed before. (Should we still permit passing a `PrimFunc` to `call_dps_packed`?)
We can enforce it in `MakeCallTIR` and `MakeCallDPSPacked`.
For now I don't see a reason to allow `call_dps_packed` to call `Primfunc`.
cc @tqchen
--
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] tqchen commented on pull request #14183: [Unity] Introduce call_dps_packed
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on PR #14183:
URL: https://github.com/apache/tvm/pull/14183#issuecomment-1455100768
please run the following commend to update to latest change
```
git rebase --onto upstream/unity upstream/unity-rebase-backup-2023-03-05
```
--
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] slyubomirsky commented on pull request #14183: [Unity] Introduce call_dps_packed
Posted by "slyubomirsky (via GitHub)" <gi...@apache.org>.
slyubomirsky commented on PR #14183:
URL: https://github.com/apache/tvm/pull/14183#issuecomment-1459001356
Yes, the RHS of that assignment would have to be a global var; we do not permit inline PrimFunc definitions. Perhaps we should get wider agreement if we want to have a restriction of using only global vars for `call_tir` or `call_dps_packed`?
--
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] yongwww commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "yongwww (via GitHub)" <gi...@apache.org>.
yongwww commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1131639173
##########
src/relax/transform/call_tir_rewrite.cc:
##########
@@ -33,7 +33,7 @@ namespace relax {
Review Comment:
Thanks for the suggestion! We can discuss it later, it might need to get wider agreement for pass renaming.
--
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] slyubomirsky commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "slyubomirsky (via GitHub)" <gi...@apache.org>.
slyubomirsky commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1123973638
##########
src/relax/op/op.cc:
##########
@@ -106,6 +106,44 @@ Expr MakeCallTIR(Expr func, Tuple args, Array<TensorStructInfo> out_sinfo_list,
TVM_REGISTER_GLOBAL("relax.op.call_tir").set_body_typed(MakeCallTIR);
+// call_dps_packed
+
+StructInfo InferStructInfoCallDPSPacked(const Call& call, const BlockBuilder& ctx) {
+ if (call->sinfo_args.size() != 1) {
+ ctx->ReportFatal(Diagnostic::Error(call)
+ << "sinfo_args should have exact 1 output struct info.");
+ }
+ return call->sinfo_args[0];
+}
+
+RELAY_REGISTER_OP("relax.call_dps_packed")
+ .set_num_inputs(2)
+ .add_argument("func", "Expr", "The destination-passing-style function.")
+ .add_argument("args", "Tuple", "The input arguments.")
+ .set_attr<FInferStructInfo>("FInferStructInfo", InferStructInfoCallDPSPacked);
+
+Expr MakeCallDPSPacked(Expr func, Tuple args, Array<TensorStructInfo> out_sinfo_list) {
Review Comment:
Why are the output StructInfo required to be tensors?
--
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] psrivas2 commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "psrivas2 (via GitHub)" <gi...@apache.org>.
psrivas2 commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1124566827
##########
src/relax/op/op.cc:
##########
@@ -106,6 +106,44 @@ Expr MakeCallTIR(Expr func, Tuple args, Array<TensorStructInfo> out_sinfo_list,
TVM_REGISTER_GLOBAL("relax.op.call_tir").set_body_typed(MakeCallTIR);
+// call_dps_packed
+
+StructInfo InferStructInfoCallDPSPacked(const Call& call, const BlockBuilder& ctx) {
+ if (call->sinfo_args.size() != 1) {
+ ctx->ReportFatal(Diagnostic::Error(call)
+ << "sinfo_args should have exact 1 output struct info.");
+ }
+ return call->sinfo_args[0];
+}
+
+RELAY_REGISTER_OP("relax.call_dps_packed")
+ .set_num_inputs(2)
+ .add_argument("func", "Expr", "The destination-passing-style function.")
+ .add_argument("args", "Tuple", "The input arguments.")
+ .set_attr<FInferStructInfo>("FInferStructInfo", InferStructInfoCallDPSPacked);
+
+Expr MakeCallDPSPacked(Expr func, Tuple args, Array<TensorStructInfo> out_sinfo_list) {
+ for (const TensorStructInfo& sinfo : out_sinfo_list) {
Review Comment:
This has a lot of common code with MakeCallTIR, can we avoid code duplication 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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] yongwww commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "yongwww (via GitHub)" <gi...@apache.org>.
yongwww commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1128697514
##########
src/relax/op/op.cc:
##########
@@ -106,6 +106,44 @@ Expr MakeCallTIR(Expr func, Tuple args, Array<TensorStructInfo> out_sinfo_list,
TVM_REGISTER_GLOBAL("relax.op.call_tir").set_body_typed(MakeCallTIR);
+// call_dps_packed
+
+StructInfo InferStructInfoCallDPSPacked(const Call& call, const BlockBuilder& ctx) {
+ if (call->sinfo_args.size() != 1) {
+ ctx->ReportFatal(Diagnostic::Error(call)
+ << "sinfo_args should have exact 1 output struct info.");
+ }
+ return call->sinfo_args[0];
+}
+
+RELAY_REGISTER_OP("relax.call_dps_packed")
+ .set_num_inputs(2)
+ .add_argument("func", "Expr", "The destination-passing-style function.")
+ .add_argument("args", "Tuple", "The input arguments.")
+ .set_attr<FInferStructInfo>("FInferStructInfo", InferStructInfoCallDPSPacked);
+
+Expr MakeCallDPSPacked(Expr func, Tuple args, Array<TensorStructInfo> out_sinfo_list) {
+ for (const TensorStructInfo& sinfo : out_sinfo_list) {
Review Comment:
it has a few lines (around 10) of common code here, probably we can keep it
--
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] psrivas2 commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "psrivas2 (via GitHub)" <gi...@apache.org>.
psrivas2 commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1131484433
##########
src/relax/transform/call_tir_rewrite.cc:
##########
@@ -33,7 +33,7 @@ namespace relax {
Review Comment:
nit: should we rename this file and pass to reflect that it handles both call_dps_packed and call_tir?
##########
src/script/printer/relax/call.cc:
##########
@@ -134,7 +138,7 @@ Optional<ExprDoc> PrintCallTIR(const relax::Call& n, const ObjectPath& n_p, cons
TVM_STATIC_IR_FUNCTOR(IRDocsifier, vtable)
.set_dispatch<relax::Call>( //
"", [](relax::Call n, ObjectPath n_p, IRDocsifier d) -> Doc {
- // Special case: call_tir
+ // Special case: call_tir, call_dps_packed
if (Optional<ExprDoc> doc = PrintCallTIR(n, n_p, d)) {
Review Comment:
nit: does PrintCallTIR need renaming?
--
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] slyubomirsky commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "slyubomirsky (via GitHub)" <gi...@apache.org>.
slyubomirsky commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1123974355
##########
tests/python/relax/test_ast_printer.py:
##########
@@ -432,7 +432,7 @@ def test_call_tir():
@R.function
def foo(x: R.Tensor(("m", "n"), "float32")):
m, n = T.var("int64"), T.var("int64")
- gv0 = R.call_tir("test.op.identity", (x,), R.Tensor((m, n), dtype="float32"))
+ gv0 = R.call_dps_packed("test.op.identity", (x,), R.Tensor((m, n), dtype="float32"))
Review Comment:
Maybe we should just drop this test case if it's not going to actually use `call_tir`
--
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] tqchen commented on pull request #14183: [Unity] Introduce call_dps_packed
Posted by "tqchen (via GitHub)" <gi...@apache.org>.
tqchen commented on PR #14183:
URL: https://github.com/apache/tvm/pull/14183#issuecomment-1454734364
We can have wellformess to enforce such invariant
--
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] yongwww commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "yongwww (via GitHub)" <gi...@apache.org>.
yongwww commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1131532774
##########
src/relax/transform/call_tir_rewrite.cc:
##########
@@ -33,7 +33,7 @@ namespace relax {
Review Comment:
I was thinking about this, but don't have a better name, any suggestions? if we change this, then the pass name CallTIRRewrite needs to be updated accordingly. We can keep it untouched for now if we don't have an alternative 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.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] slyubomirsky commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "slyubomirsky (via GitHub)" <gi...@apache.org>.
slyubomirsky commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1128634605
##########
include/tvm/relax/transform.h:
##########
@@ -89,6 +89,13 @@ TVM_DLL Pass ToNonDataflow();
*/
TVM_DLL Pass CallTIRRewrite();
+/*!
+ * \brief Perform explicit tensor allocation for call_dps_packed.
+ *
+ * \return The Pass.
+ */
+TVM_DLL Pass CallDPSPackedRewrite();
Review Comment:
It might be fine to make them syntactic sugar for the same underlying operator. There are only two differences I can think of:
1. `call_tir` is generally assumed to be pure, whereas `call_dps_packed` may not be (unless we want to require it)
2. `call_tir` expects a `PrimFunc` as its operand, whereas `call_dps_packed` expects a `PackedFunc`.
--
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] slyubomirsky commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "slyubomirsky (via GitHub)" <gi...@apache.org>.
slyubomirsky commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1128634605
##########
include/tvm/relax/transform.h:
##########
@@ -89,6 +89,13 @@ TVM_DLL Pass ToNonDataflow();
*/
TVM_DLL Pass CallTIRRewrite();
+/*!
+ * \brief Perform explicit tensor allocation for call_dps_packed.
+ *
+ * \return The Pass.
+ */
+TVM_DLL Pass CallDPSPackedRewrite();
Review Comment:
It might be fine to make them syntactic sugar for the same underlying operator. There are only two differences I can think of:
1. `call_tir` is generally assumed to be pure, whereas `call_dps_packed` may not be (unless we want to require it)
2. `call_tir` expects a `PrimFunc` as its operand, whereas `call_dps_packed` expects a `PackedFunc`.
The first can be enforced through the StructInfo system. The second, though, would need to be enforced dynamically in the general case
##########
include/tvm/relax/transform.h:
##########
@@ -89,6 +89,13 @@ TVM_DLL Pass ToNonDataflow();
*/
TVM_DLL Pass CallTIRRewrite();
+/*!
+ * \brief Perform explicit tensor allocation for call_dps_packed.
+ *
+ * \return The Pass.
+ */
+TVM_DLL Pass CallDPSPackedRewrite();
Review Comment:
It might be fine to make them syntactic sugar for the same underlying operator. There are only two differences I can think of:
1. `call_tir` is generally assumed to be pure, whereas `call_dps_packed` may not be (unless we want to require it)
2. `call_tir` expects a `PrimFunc` as its operand, whereas `call_dps_packed` expects a `PackedFunc`.
The first can be enforced through the StructInfo system. The second, though, would need to be enforced dynamically in the general case
--
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] slyubomirsky merged pull request #14183: [Unity] Introduce call_dps_packed
Posted by "slyubomirsky (via GitHub)" <gi...@apache.org>.
slyubomirsky merged PR #14183:
URL: https://github.com/apache/tvm/pull/14183
--
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] slyubomirsky commented on pull request #14183: [Unity] Introduce call_dps_packed
Posted by "slyubomirsky (via GitHub)" <gi...@apache.org>.
slyubomirsky commented on PR #14183:
URL: https://github.com/apache/tvm/pull/14183#issuecomment-1452877924
Do we have any way to enforce that the function passed to `call_tir` is a `PrimFunc`? I believe making `call_tir` more specific was discussed before. (Should we still permit passing a `PrimFunc` to `call_dps_packed`?)
--
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] yongwww commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "yongwww (via GitHub)" <gi...@apache.org>.
yongwww commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1128704973
##########
include/tvm/relax/transform.h:
##########
@@ -89,6 +89,13 @@ TVM_DLL Pass ToNonDataflow();
*/
TVM_DLL Pass CallTIRRewrite();
+/*!
+ * \brief Perform explicit tensor allocation for call_dps_packed.
+ *
+ * \return The Pass.
+ */
+TVM_DLL Pass CallDPSPackedRewrite();
Review Comment:
Thanks for the comments! Actually I was struggling to add CallDPSPackedRewrite, it does duplicate lots of code from CallTIRRewrite, and modifying CallTIRRewrite to work for both call_tir and call_dps_packed brings some confusion (probably a different pass name). I removed CallDPSPackedRewrite in the latest commit, we can add it back when the differences are introduces.
--
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] slyubomirsky commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "slyubomirsky (via GitHub)" <gi...@apache.org>.
slyubomirsky commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1128866691
##########
src/relax/analysis/well_formed.cc:
##########
@@ -260,6 +261,19 @@ class WellFormedChecker : public relax::ExprVisitor,
}
}
+ // call_tir works for tir PrimFunc, call_dps_packed works for ExternFunc
+ static const Op& call_tir_op = Op::Get("relax.call_tir");
+ static const Op& call_dps_packed_op = Op::Get("relax.call_dps_packed");
+ if (op->op == call_tir_op && !op->args[0]->IsInstance<GlobalVarNode>()) {
+ Malformed(Diagnostic::Error(op->args[0]->span)
+ << "call_tir expects a prim_func, but gets " << op->args[0]->GetTypeKey());
+ }
Review Comment:
As mentioned in the community meeting, we can enforce this in the `FInferStructInfo` for `call_tir` and not need any special cases in the well-formedness check.
--
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] yongwww commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "yongwww (via GitHub)" <gi...@apache.org>.
yongwww commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1131532774
##########
src/relax/transform/call_tir_rewrite.cc:
##########
@@ -33,7 +33,7 @@ namespace relax {
Review Comment:
I was thinking about this, but don't have a better name, any suggestions? if we change this, then the pass name CallTIRRewrite needs to be updated accordingly.
--
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] yongwww commented on pull request #14183: [Unity] Introduce call_dps_packed
Posted by "yongwww (via GitHub)" <gi...@apache.org>.
yongwww commented on PR #14183:
URL: https://github.com/apache/tvm/pull/14183#issuecomment-1462841675
> Overall LGTM! Thanks! Just some minor nits which can be handled in follow up PRs
>
> legalize_ops would throw a warning for call_dps_packed: https://github.com/psrivas2/tvm/blob/emit-te/src/relax/transform/legalize_ops.cc#L105
>
> Would there be follow up PRs for cleaning up transform/backend passes which earlier checked if call_tir is calling an extern func such as
>
> * https://github.com/psrivas2/tvm/blob/emit-te/src/relax/backend/task_extraction.cc#L77
> * https://github.com/psrivas2/tvm/blob/emit-te/src/relax/transform/fold_constant.cc#L181
> * https://github.com/psrivas2/tvm/blob/emit-te/src/relax/transform/fuse_ops.cc#L188
> * https://github.com/psrivas2/tvm/blob/emit-te/src/relax/transform/fuse_tir.cc#L305
> * https://github.com/psrivas2/tvm/blob/emit-te/src/relax/transform/rewrite_dataflow_reshape.cc#L78
Have updated for all passes, thanks a lot for pointing them out!!!
--
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] psrivas2 commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "psrivas2 (via GitHub)" <gi...@apache.org>.
psrivas2 commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1131636406
##########
src/relax/transform/call_tir_rewrite.cc:
##########
@@ -33,7 +33,7 @@ namespace relax {
Review Comment:
DPSRewrite would be my choice but we can discuss and change it later.
--
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] yongwww commented on pull request #14183: [Unity] Introduce call_dps_packed
Posted by "yongwww (via GitHub)" <gi...@apache.org>.
yongwww commented on PR #14183:
URL: https://github.com/apache/tvm/pull/14183#issuecomment-1458996643
> It would not always be possible to check statically that the operand to `call_tir` is a `PrimFunc`. Here is a case where it wouldn't work:
>
> ```python
> @R.function
> def higher_order(f: R.Object) -> R.Object:
> return R.call_tir(f, ...)
>
> @R.function
> def g() -> R.Object:
> if some_condition:
> y = some_packed_func
> else:
> y = some_primfunc
> return higher_order(y)
> ```
>
> We would not be able to determine statically that the call to `higher_order` in `g` will result in a non-`PrimFunc` being passed to `call_tir`. If we care about enforcing the distinction, we would have to check for such cases dynamically.
>
> Edit: Otherwise, we could ask for only global vars to be the arguments to `call_tir`, which I don't think would be the worst thing in the world, but this limits the extent to which `PrimFunc`s and `PackedFunc`s are first class. (I don't think they really need to be first class, but in past discussions, we've said that that was desirable.)
Have updated to well-form check to just allow GlobalVar as the first argument of call_tir. Actually current inline PrimFunc was disallowed in Relax IR, `y = some_primfunc` in the example has to be `y = global_var_of_some_primfunc`. I guess we won't run into issues at this moment, correct me if i am wrong
--
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] yongwww commented on pull request #14183: [Unity] Introduce call_dps_packed
Posted by "yongwww (via GitHub)" <gi...@apache.org>.
yongwww commented on PR #14183:
URL: https://github.com/apache/tvm/pull/14183#issuecomment-1459010598
> Yes, the RHS of that assignment would have to be a global var; we do not permit inline PrimFunc definitions. Perhaps we should get wider agreement if we want to have a restriction of using only global vars for `call_tir` or `call_dps_packed`? I can update the spec to reflect whatever we decide.
Perhaps we can discuss this in the open dev meeting today if time allows
--
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] slyubomirsky commented on pull request #14183: [Unity] Introduce call_dps_packed
Posted by "slyubomirsky (via GitHub)" <gi...@apache.org>.
slyubomirsky commented on PR #14183:
URL: https://github.com/apache/tvm/pull/14183#issuecomment-1458925793
It would not always be possible to check statically that the operand to `call_tir` is a `PrimFunc`. Here is a case where it wouldn't work:
```python
@R.function
def higher_order(f: R.Object) -> R.Object:
return R.call_tir(f, ...)
@R.function
def g() -> R.Object:
if some_condition:
y = some_packed_func
else:
y = some_primfunc
return higher_order(y)
```
We would not be able to determine statically that the call to `higher_order` in `g` will result in a non-`PrimFunc` being passed to `call_tir`. If we care about enforcing the distinction, we would have to check for such cases dynamically.
--
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] slyubomirsky commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "slyubomirsky (via GitHub)" <gi...@apache.org>.
slyubomirsky commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1130045807
##########
tests/python/relax/test_ast_printer.py:
##########
@@ -432,7 +432,7 @@ def test_call_tir():
@R.function
def foo(x: R.Tensor(("m", "n"), "float32")):
m, n = T.var("int64"), T.var("int64")
- gv0 = R.call_tir("test.op.identity", (x,), R.Tensor((m, n), dtype="float32"))
+ gv0 = R.call_dps_packed("test.op.identity", (x,), R.Tensor((m, n), dtype="float32"))
Review Comment:
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] MasterJH5574 commented on a diff in pull request #14183: [Unity] Introduce call_dps_packed
Posted by "MasterJH5574 (via GitHub)" <gi...@apache.org>.
MasterJH5574 commented on code in PR #14183:
URL: https://github.com/apache/tvm/pull/14183#discussion_r1125354651
##########
include/tvm/relax/transform.h:
##########
@@ -89,6 +89,13 @@ TVM_DLL Pass ToNonDataflow();
*/
TVM_DLL Pass CallTIRRewrite();
+/*!
+ * \brief Perform explicit tensor allocation for call_dps_packed.
+ *
+ * \return The Pass.
+ */
+TVM_DLL Pass CallDPSPackedRewrite();
Review Comment:
I’m wondering if we need this pass. I suppose that the lowering of `call_tir` and `call_dps_packed` are the same (the diff check also suggests this). The reason we want to introduce two is to reduce confusion. In terms of their lowering, I suppose they can directly share a same logic, which makes things easy (say when someone wants to change CallTIRRewrite, it won’t need to change CallDPSPackedRewrite in the same way as well).
--
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