Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Relay][VM]Compiling pattern matching #3470

Merged
merged 20 commits into from
Jul 9, 2019
Merged

[Relay][VM]Compiling pattern matching #3470

merged 20 commits into from
Jul 9, 2019

Conversation

wweic
Copy link
Contributor

@wweic wweic commented Jul 1, 2019 •
edited
Loading

  1. Compile pattern match expression into decision tree, and emit VM byte code from the tree.
  2. Add new opcode.
    1. GetTag: Get the object tag and stores the TensorCell object.
    2. Loadconsti: Load a primitive int value into a TensorCell
    3. Fatal: Fail the VM execution
  3. Update existing opcdeo:
    1. Ifi: It tests 2 register test and target, if they are equal, execute then branch, otherwise else branch.
    2. Selecti: It tests 2 integer register test and target, if equal, return op1, otherwise op2.

cc: @jroesch @MarisaKirisame @icemelon9 @zhiics @slyubomirsky @junrushao1994

@slyubomirsky
Copy link
Contributor

slyubomirsky commented Jul 2, 2019 •
edited
Loading

I don't have any issues with these changes (seems like a reasonable approach to supporting matching), though granted I had not previously closely followed VM development. Glad to see all the tests added.

MarisaKirisame
MarisaKirisame reviewed Jul 2, 2019
View reviewed changes
src/runtime/vm/vm.cc Outdated Show resolved Hide resolved
src/runtime/vm/vm.cc Outdated Show resolved Hide resolved
@wweic wweic force-pushed the matching branch 2 times, most recently from 9f668b0 to 3ef91a8 Compare July 2, 2019 22:11
@MarisaKirisame
Copy link
Contributor

How hard is it to refactor the decision tree into a common api?

@wweic
Copy link
Contributor Author

wweic commented Jul 2, 2019

@MarisaKirisame It's easy to move out ConditionNode and TreeNode out to a header. We just need to decide where the header file should be.

@MarisaKirisame
Copy link
Contributor

MarisaKirisame commented Jul 2, 2019 •
edited
Loading

@wweic into pass_util? It is something people can use when writing pass.

@jroesch
Copy link
Member

jroesch commented Jul 3, 2019

Why not just use tensors? we could instead specialize for the 0-rank case behind the scenes. I'm wary of opcode bloat and proliferation of data types.

@wweic
Copy link
Contributor Author

wweic commented Jul 3, 2019

@jroesch Thanks for the comment. I'm not sure as well. I think I can get rid of Ifi and Selecti. As for Cmpi, are you ok with assuming the tensor is 0 rank? Or implement a general tensor comparison?

zhiics
zhiics reviewed Jul 3, 2019
View reviewed changes
Copy link
Member

@zhiics zhiics left a comment

Choose a reason for hiding this comment

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

LGTM

zhiics
zhiics reviewed Jul 3, 2019
View reviewed changes
src/runtime/vm/vm.cc Outdated Show resolved Hide resolved
@jroesch
Copy link
Member

jroesch commented Jul 5, 2019

@jroesch Thanks for the comment. I'm not sure as well. I think I can get rid of Ifi and Selecti. As for Cmpi, are you ok with assuming the tensor is 0 rank? Or implement a general tensor comparison?

Maybe we can generalize the boolean comparison instruction? maybe we do need to type it though

MarisaKirisame
MarisaKirisame reviewed Jul 7, 2019
View reviewed changes
src/relay/backend/vm/compiler.cc Outdated Show resolved Hide resolved
src/relay/pass/pass_util.h Show resolved Hide resolved
@jroesch jroesch merged commit 93d1c06 into apache:master Jul 9, 2019
wweic added a commit to wweic/tvm that referenced this pull request Jul 11, 2019
@wweic
[Relay][VM]Compiling pattern matching (apache#3470)
9178af7 
* [Relay][VM]Compiling pattern matching

* Fix lint

* Remove debug code

* Move TreeNode definition

* merge ifi and selecti, todo: remove them

* fix lint

* remove ifi and selecti

* rename GetTagi to GetTag

* fix dltype

* fix more dltype

* Generalize If and select, and rename to Ifi and Selecti

* Fix lint

* Rename Ifi to If

* Change register default to match value

* Remove bad specialization for Move

* Stop use Select

* Remove Select

* TreeNode refactor

* Change entry_func name

* Remove Cmp due to rebase issue
wweic added a commit to wweic/tvm that referenced this pull request Jul 11, 2019
@wweic
[Relay][VM]Compiling pattern matching (apache#3470)
c84a276 
* [Relay][VM]Compiling pattern matching

* Fix lint

* Remove debug code

* Move TreeNode definition

* merge ifi and selecti, todo: remove them

* fix lint

* remove ifi and selecti

* rename GetTagi to GetTag

* fix dltype

* fix more dltype

* Generalize If and select, and rename to Ifi and Selecti

* Fix lint

* Rename Ifi to If

* Change register default to match value

* Remove bad specialization for Move

* Stop use Select

* Remove Select

* TreeNode refactor

* Change entry_func name

* Remove Cmp due to rebase issue
wweic added a commit to neo-ai/tvm that referenced this pull request Jul 11, 2019
@wweic
[Relay][VM]Compiling pattern matching (apache#3470)
83c067a 
* [Relay][VM]Compiling pattern matching

* Fix lint

* Remove debug code

* Move TreeNode definition

* merge ifi and selecti, todo: remove them

* fix lint

* remove ifi and selecti

* rename GetTagi to GetTag

* fix dltype

* fix more dltype

* Generalize If and select, and rename to Ifi and Selecti

* Fix lint

* Rename Ifi to If

* Change register default to match value

* Remove bad specialization for Move

* Stop use Select

* Remove Select

* TreeNode refactor

* Change entry_func name

* Remove Cmp due to rebase issue
@wweic wweic deleted the matching branch July 18, 2019 19:57
@tqchen tqchen mentioned this pull request Nov 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhiics zhiics zhiics left review comments

@MarisaKirisame MarisaKirisame MarisaKirisame approved these changes

@jroesch jroesch jroesch approved these changes

Assignees
No one assigned
Labels
status: need review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@wweic @slyubomirsky @MarisaKirisame @jroesch @zhiics @yzhliu