Skip to content

[Unity][TVMScript] Fix prim_func lost issue in relax.emit_te#14189

Merged
Hzfengsy merged 4 commits intoapache:unityfrom
yongwww:fix-parser-emit-te
Mar 8, 2023
Merged

[Unity][TVMScript] Fix prim_func lost issue in relax.emit_te#14189
Hzfengsy merged 4 commits intoapache:unityfrom
yongwww:fix-parser-emit-te

Conversation

@yongwww
Copy link
Member

@yongwww yongwww commented Mar 4, 2023

Previously R.emit_te was introduced in #14123. The prim_funcs were not added into the same ir_module. The pr is to fix this issue, move some call_tir input handling code from bb.call_te to utils, then it is able to be leveraged by both bb.emit_te and R.emit_te.

cc: @psrivas2 @tqchen

@tvm-bot
Copy link
Collaborator

tvm-bot commented Mar 4, 2023

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 by @-ing them in a comment.

Generated by tvm-bot

@yongwww yongwww force-pushed the fix-parser-emit-te branch from 4f59131 to 1bd78b4 Compare March 4, 2023 06:03
@tqchen
Copy link
Member

tqchen commented Mar 5, 2023

please run the following commend to update to latest change

git rebase --onto upstream/unity upstream/unity-rebase-backup-2023-03-05 

@yongwww yongwww force-pushed the fix-parser-emit-te branch from 1bd78b4 to 16be499 Compare March 8, 2023 00:58
@yongwww
Copy link
Member Author

yongwww commented Mar 8, 2023

Have fixed the comments, pls take another look. cc @Hzfengsy

}
}

GlobalVar AddFunction(const BaseFunc& func, String func_name_hint) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between these two functions and DeclFunction, DefFunction?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main difference is FindModuleFrame used in DeclFunction and DefFunction, the two guys are required to be called immediately under I.ir_module(). AddFunction and UpdateFunction don't have this restriction.

Tried to use decl_function in emit_te, ran into error ValueError: 'I.DeclFunction' must be called immediately under I.ir_module()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you enhance those two instead of writing two new ones?

Excaully ValueError: 'I.DeclFunction' must be called immediately under I.ir_module() is not necessary in this case

Copy link
Member Author

@yongwww yongwww Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, thanks for letting me know that! Have removed add/update_function then. pls take another look

Copy link
Contributor

@psrivas2 psrivas2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @yongwww !

@Hzfengsy Hzfengsy merged commit 9879263 into apache:unity Mar 8, 2023
tqchen pushed a commit that referenced this pull request Mar 13, 2023
Previously R.emit_te was introduced in #14123. The `prim_func`s were not added into the same `ir_module`. The pr is to fix this issue, move some `call_tir` input handling code from `bb.call_te` to utils, then it is able to be leveraged by both `bb.emit_te` and `R.emit_te`.
tqchen pushed a commit that referenced this pull request Mar 13, 2023
Previously R.emit_te was introduced in #14123. The `prim_func`s were not added into the same `ir_module`. The pr is to fix this issue, move some `call_tir` input handling code from `bb.call_te` to utils, then it is able to be leveraged by both `bb.emit_te` and `R.emit_te`.
tqchen pushed a commit that referenced this pull request Mar 13, 2023
Previously R.emit_te was introduced in #14123. The `prim_func`s were not added into the same `ir_module`. The pr is to fix this issue, move some `call_tir` input handling code from `bb.call_te` to utils, then it is able to be leveraged by both `bb.emit_te` and `R.emit_te`.
tqchen pushed a commit that referenced this pull request Mar 13, 2023
Previously R.emit_te was introduced in #14123. The `prim_func`s were not added into the same `ir_module`. The pr is to fix this issue, move some `call_tir` input handling code from `bb.call_te` to utils, then it is able to be leveraged by both `bb.emit_te` and `R.emit_te`.
tqchen pushed a commit that referenced this pull request Mar 20, 2023
Previously R.emit_te was introduced in #14123. The `prim_func`s were not added into the same `ir_module`. The pr is to fix this issue, move some `call_tir` input handling code from `bb.call_te` to utils, then it is able to be leveraged by both `bb.emit_te` and `R.emit_te`.
@yongwww yongwww deleted the fix-parser-emit-te branch March 23, 2023 23:46
tqchen pushed a commit that referenced this pull request Apr 1, 2023
Previously R.emit_te was introduced in #14123. The `prim_func`s were not added into the same `ir_module`. The pr is to fix this issue, move some `call_tir` input handling code from `bb.call_te` to utils, then it is able to be leveraged by both `bb.emit_te` and `R.emit_te`.
tqchen pushed a commit that referenced this pull request Apr 1, 2023
Previously R.emit_te was introduced in #14123. The `prim_func`s were not added into the same `ir_module`. The pr is to fix this issue, move some `call_tir` input handling code from `bb.call_te` to utils, then it is able to be leveraged by both `bb.emit_te` and `R.emit_te`.
tqchen pushed a commit that referenced this pull request Apr 1, 2023
Previously R.emit_te was introduced in #14123. The `prim_func`s were not added into the same `ir_module`. The pr is to fix this issue, move some `call_tir` input handling code from `bb.call_te` to utils, then it is able to be leveraged by both `bb.emit_te` and `R.emit_te`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants