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

[RUST] RFC and 0.5 Release Plans #2306

Closed
5 tasks done
ehsanmok opened this issue Dec 18, 2018 · 4 comments · Fixed by #2292
Closed
5 tasks done

[RUST] RFC and 0.5 Release Plans #2306

ehsanmok opened this issue Dec 18, 2018 · 4 comments · Fixed by #2292

Comments

@ehsanmok
Copy link
Contributor

ehsanmok commented Dec 18, 2018

The following are the current plans for TVM Rust including both "runtime" and "frontend" as part of roadmap v0.5.

  • Create common crate for runtime and frontend
  • Merge frontend
  • Comply with Rust 2018-edition.
  • Add function builder to runtime and make runtime PackedFunc compatible with frontend
  • Make call_packed! macro more general and compatible across both runtime and frontend sides as discussed.
@ehsanmok ehsanmok changed the title [RUST] comply with 2018 edition [RUST] RFC and Release Plans Dec 24, 2018
@ehsanmok
Copy link
Contributor Author

ehsanmok commented Jan 5, 2019

Here're my thoughts about a common crate:

The main difference (besides some conversions and using a fat point) between frontend and runtime TVMArgValue and TVMRetValue is how they're wrapping/using TVMValue. The runtime TVMValue is raw, however, frontend needs Debug + Display and some idiomatic conversions. Therefore, common's TVMValue has to be wrapped at least.

Because bindgen doesn't work for runtime, so either we should

  • include the generated raw c api in common entirely (same as runtime)
    or
  • include what needed such as raw TVMValue and TVMTypeCode and then, wrap and impl required conversions for both runtime and frontend (because of Orphan rule so that we won't need more wrapper boilerplates in either crates).

@ehsanmok
Copy link
Contributor Author

ehsanmok commented Jan 8, 2019

@nhynes I'm working on the common crate and it's completely non-obvious how to make it. I'm making minimum assumptions like even removing the debugging for TVMValue and including it raw same as in Rust runtime. The only idea that might lead to a solution is to include the entire c_runtime_api.rs in common and get rid of my tvm-sys which I don't like and it would impose very uncommon layout for a Rust binding. If I include some partial C runtime api such as TVMValue and TVMTypeCode, then Rust's orphan rule prevents me from conversion from common::TVMValue to tvm_sys::TVMValue and need to change all occurences of tvm_sys::TVMValue in the raw api to common::TVMValue which is not possible!

I'm now thinking maybe common crate is not a good idea here and we can have exactly the same functionalities with two different implementations because runtime and frontend needs are different when we include debugging for example. Note that this includes very basic value conversions API only and it's not significant.

Overall, I'll work on making a common crate this week and next maybe and if I wouldn't have found a solution by then, I'd just change the frontend TVMRetValue to exactly match the runtime and impl the same conversions and send the PR.

Any comment?

@nhynes
Copy link
Member

nhynes commented Jan 8, 2019

an interesting point. what happens if the common crate has the headers behind one of two mutually exclusive feature flags: runtime and frontend? The former would pull in the definitions from c_runtime_api.rs and the latter would expose tvm-sys.

removing the debugging

I'm not sure I follow. Is it not as easy as #[derive(Debug)] or even impl Debug for Struct

Like even if we have to do out-of-band codegen, I think that it's worth having one API. If we don't, SWIM will surely come by in a few months, read the codebase, and think "wtf. why are there two different implementations of the same thing?!?!" much as I do when reading topi code :P

I'd prefer for you to not struggle with this, so if it really becomes too onerous, just give me your acceptance criteria for the frontend "working" and I'll do my best to merge them.

@ehsanmok
Copy link
Contributor Author

ehsanmok commented Jan 10, 2019

what happens if the common crate has the headers behind one of two mutually exclusive feature flags: runtime and frontend?

Well, I haven't thought about that. It seems a viable approach.

I'm not sure I follow. Is it not as easy as #[derive(Debug)] or even impl Debug for Struct

I take back my debugging concern. Previously, I sort of made TVMValue into PartialEq + Eq by attaching a type but is not necessary and it should be fine to just impl Debug.

I'd prefer for you to not struggle with this, so if it really becomes too onerous, just give me your acceptance criteria for the frontend "working" and I'll do my best to merge them.

Thanks for the support 👍

Right now, I'm using runtime impl in common with some changes in runtime. I'll update you for the frontend compatibility as it implies some good number of changes as well. Hopefully it won't be too much headache!

I should say that inevitably the downside of common is exposing all fields of TVMArgValue (include _lifetime) and TVMRetValue which might be ok for our case but need more care perhaps!

@nhynes nhynes changed the title [RUST] RFC and Release Plans [RUST] RFC and 0.5 Release Plans Jan 29, 2019
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 a pull request may close this issue.

2 participants