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.

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

[Relax] Support callback as argument #16542

Merged
merged 2 commits into from
Feb 13, 2024

Conversation

Lunderberg
Copy link
Contributor

Prior to this commit, calls from Relax to external PackedFuncs could only be done through the TVM global registry. While Relax functions accepting a callback could be written as callback_arg: R.Callable(arg_struct_info, ret_struct_info), attempting to compile these functions would raise an error during the CodeGenVM step of relax.build. In addition, the global registry is only queried when initializing the relax.VirtualMachine, and so later changes requires restarting the VM.

This commit updates both the CodeGenVM lowering pass and the relax VM to support callbacks. The motivating use case is with the LazyTransformParams pass, to improve flexibility by avoiding use of the global registry.

Prior to this commit, calls from Relax to external PackedFuncs could
only be done through the TVM global registry.  While Relax functions
accepting a callback could be written as `callback_arg:
R.Callable(arg_struct_info, ret_struct_info)`, attempting to compile
these functions would raise an error during the `CodeGenVM` step of
`relax.build`.  In addition, the global registry is only queried when
initializing the `relax.VirtualMachine`, and so later changes requires
restarting the VM.

This commit updates both the `CodeGenVM` lowering pass and the relax
VM to support callbacks.  The is primarily intended for use with the
`LazyTransformParams` pass, to improve flexibility by avoiding use of
the global registry.
@Lunderberg Lunderberg force-pushed the relax_vm_callback_argument branch from a264e81 to f34323b Compare February 8, 2024 12:44
@Lunderberg
Copy link
Contributor Author

Merged from main to PR branch, to resolve CI breakage that was fixed in #16546.

Copy link
Contributor

@slyubomirsky slyubomirsky left a comment

Choose a reason for hiding this comment

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

The change seems straightforward and it's good that there are some test cases too. I'm glad that this case will work now.

@Lunderberg Lunderberg merged commit 3c5ee30 into apache:main Feb 13, 2024
19 checks passed
@Lunderberg Lunderberg deleted the relax_vm_callback_argument branch February 13, 2024 18:07
/*! \brief The index into the packed function table. */
/*! \brief The index of the function.
*
* For `OpCode::Call`, this is an index into the table of static
Copy link
Member

Choose a reason for hiding this comment

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

we should not use CallFromRegister, instead we should lower to https://github.com/apache/tvm/blob/main/src/runtime/relax_vm/builtin.cc#L373

@tqchen
Copy link
Member

tqchen commented Feb 14, 2024

Sorry for being late on this one. In this particular case, it is better to use vm.builtin.invoke_closure to lower those possible variables that contains the Callable. There are two reasons for this

  • One goal of relax VM is to ensure minimal set of instructions, so far we have been keeping the miminum variant via Call and all the rest call variant using builtins like vm.builtin.invoke_closure
  • We need to handle the case where callback is either a PackedFunc or Closure, invoke_closure handles the case. This enables reuse for compiled cases

@Lunderberg would be great to followup on this soon given this would change some binary format of the VM https://github.com/apache/tvm/blob/main/src/runtime/relax_vm/builtin.cc#L373

@tqchen
Copy link
Member

tqchen commented Feb 14, 2024

cc @yongwww as well

@Lunderberg
Copy link
Contributor Author

@tqchen Thank you, and I'll make a follow-up PR. I had thought that vm.builtin.invoke_closure was only for use with internally-generated closure objects.

Lunderberg added a commit to Lunderberg/tvm that referenced this pull request Feb 14, 2024
A follow-up PR from apache#16542, after
some post-merge discussion regarding the implementation.
@Lunderberg
Copy link
Contributor Author

I've submitted #16573, which updates the handling of callback functions, reverts the VM changes, and passes the unit tests that were added in this PR.

tqchen pushed a commit that referenced this pull request Feb 15, 2024
A follow-up PR from #16542, after
some post-merge discussion regarding the implementation.
@slyubomirsky
Copy link
Contributor

Good to know re: invoke_closure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants