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/19 07:23:57 UTC
[GitHub] [incubator-tvm] windclarion opened a new pull request #5623: root->args[i] isn't a CallNode sometimes
windclarion opened a new pull request #5623:
URL: https://github.com/apache/incubator-tvm/pull/5623
root->args[i] maybe not a CallNode, so Downcast<Call>(root->args[i]) cause check fail and abort.
Signed-off-by: windclarion <wi...@gmail.com>
----------------------------------------------------------------
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] masahi commented on pull request #5623: root->args[i] isn't a CallNode sometimes
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #5623:
URL: https://github.com/apache/incubator-tvm/pull/5623#issuecomment-631174137
Thanks @windclarion
----------------------------------------------------------------
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] masahi commented on a change in pull request #5623: root->args[i] isn't a CallNode sometimes
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #5623:
URL: https://github.com/apache/incubator-tvm/pull/5623#discussion_r427262080
##########
File path: src/relay/transforms/merge_composite.cc
##########
@@ -122,8 +122,10 @@ class MergeCompositeWrapper : public ExprMutator {
for (const auto& arg : pattern->args) {
Expr new_arg;
if (arg->IsInstance<CallNode>()) {
- new_arg =
- ExtractPattern(Downcast<Call>(arg), Downcast<Call>(root->args[i]), var_map, call_map);
+ if (root->args[i]->IsInstance<CallNode>()) {
Review comment:
Move this condition to the `if` in L124.
```if (arg->IsInstance<CallNode>() && root->args[i]->IsInstance<CallNode>()) { ... }```
----------------------------------------------------------------
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] windclarion commented on a change in pull request #5623: root->args[i] isn't a CallNode sometimes
Posted by GitBox <gi...@apache.org>.
windclarion commented on a change in pull request #5623:
URL: https://github.com/apache/incubator-tvm/pull/5623#discussion_r427268341
##########
File path: src/relay/transforms/merge_composite.cc
##########
@@ -122,8 +122,10 @@ class MergeCompositeWrapper : public ExprMutator {
for (const auto& arg : pattern->args) {
Expr new_arg;
if (arg->IsInstance<CallNode>()) {
- new_arg =
- ExtractPattern(Downcast<Call>(arg), Downcast<Call>(root->args[i]), var_map, call_map);
+ if (root->args[i]->IsInstance<CallNode>()) {
Review comment:
if move to L124, then L133 "call_map->Set(arg, new_arg)" will not be executed. I'm not sure if we can bypass the L133 when arg is CallNode and root->args[i] is not CallNode.
----------------------------------------------------------------
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] masahi commented on a change in pull request #5623: root->args[i] isn't a CallNode sometimes
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #5623:
URL: https://github.com/apache/incubator-tvm/pull/5623#discussion_r427270784
##########
File path: src/relay/transforms/merge_composite.cc
##########
@@ -122,8 +122,10 @@ class MergeCompositeWrapper : public ExprMutator {
for (const auto& arg : pattern->args) {
Expr new_arg;
if (arg->IsInstance<CallNode>()) {
- new_arg =
- ExtractPattern(Downcast<Call>(arg), Downcast<Call>(root->args[i]), var_map, call_map);
+ if (root->args[i]->IsInstance<CallNode>()) {
Review comment:
if `root->args[i]->IsInstance<CallNode>()` is false, `new_arg` will be a null pointer. There is no point in caching null pointer.
----------------------------------------------------------------
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] masahi commented on a change in pull request #5623: root->args[i] isn't a CallNode sometimes
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #5623:
URL: https://github.com/apache/incubator-tvm/pull/5623#discussion_r427271984
##########
File path: src/relay/transforms/merge_composite.cc
##########
@@ -122,8 +122,10 @@ class MergeCompositeWrapper : public ExprMutator {
for (const auto& arg : pattern->args) {
Expr new_arg;
if (arg->IsInstance<CallNode>()) {
- new_arg =
- ExtractPattern(Downcast<Call>(arg), Downcast<Call>(root->args[i]), var_map, call_map);
+ if (root->args[i]->IsInstance<CallNode>()) {
Review comment:
Ideally you should add a test case to verify my claim, but since this change is trivial I'm ok with merging without a test.
----------------------------------------------------------------
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] windclarion commented on a change in pull request #5623: root->args[i] isn't a CallNode sometimes
Posted by GitBox <gi...@apache.org>.
windclarion commented on a change in pull request #5623:
URL: https://github.com/apache/incubator-tvm/pull/5623#discussion_r427319188
##########
File path: src/relay/transforms/merge_composite.cc
##########
@@ -122,8 +122,10 @@ class MergeCompositeWrapper : public ExprMutator {
for (const auto& arg : pattern->args) {
Expr new_arg;
if (arg->IsInstance<CallNode>()) {
- new_arg =
- ExtractPattern(Downcast<Call>(arg), Downcast<Call>(root->args[i]), var_map, call_map);
+ if (root->args[i]->IsInstance<CallNode>()) {
Review comment:
OK, I have modified.
----------------------------------------------------------------
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] windclarion commented on a change in pull request #5623: root->args[i] isn't a CallNode sometimes
Posted by GitBox <gi...@apache.org>.
windclarion commented on a change in pull request #5623:
URL: https://github.com/apache/incubator-tvm/pull/5623#discussion_r427268341
##########
File path: src/relay/transforms/merge_composite.cc
##########
@@ -122,8 +122,10 @@ class MergeCompositeWrapper : public ExprMutator {
for (const auto& arg : pattern->args) {
Expr new_arg;
if (arg->IsInstance<CallNode>()) {
- new_arg =
- ExtractPattern(Downcast<Call>(arg), Downcast<Call>(root->args[i]), var_map, call_map);
+ if (root->args[i]->IsInstance<CallNode>()) {
Review comment:
if move to L124, then L133 "call_map->Set(arg, new_arg)" will not be executed. I'm not sure if we can bypass the L133 if arg is CallNode and root->args[i] is not CallNode.
----------------------------------------------------------------
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] masahi merged pull request #5623: root->args[i] isn't a CallNode sometimes
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #5623:
URL: https://github.com/apache/incubator-tvm/pull/5623
----------------------------------------------------------------
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