Rustify PackedFunc & Friends #2969

nhynes · 2019-04-05T04:33:58Z

No description provided.

nhynes · 2019-04-05T06:45:17Z

cc @ehsanmok for review

Aren't PRs that have more red than green the best?

ehsanmok

Nice job! I really like the enum POD repr. Here're some small comments:

rust/common/src/packed_func.rs

ehsanmok · 2019-04-05T18:06:34Z

rust/common/src/packed_func.rs

-    pub value: TVMValue,
-    pub type_code: i64,
+/// Constructs a derivative of a TVMPodValue.
+macro_rules! TVMPODValue {


ehsanmok · 2019-04-05T18:12:47Z

rust/frontend/src/function.rs

@@ -195,11 +195,11 @@ impl<'a, 'm> Builder<'a, 'm> {
        let (mut values, mut type_codes): (Vec<ffi::TVMValue>, Vec<ffi::TVMTypeCode>) = self
            .arg_buf
            .iter()
-            .map(|tvm_arg| (tvm_arg.value, tvm_arg.type_code as ffi::TVMTypeCode))
+            .map(|arg| arg.clone().into_tvm_value())


Can it be without clone after into_iter or something?

that would require

- type PackedFunc = Fn(&[TVMArgValue]) -> TVMRetValue; + type PackedFunc = Fn(Vec<TVMArgValue>) -> TVMRetValue;

and the TVM module is surely not going to know how to Drop a Vec (nor can the frontend or runtime Drop a C++ std::vector).

I think that a better solution might be .as_tvm_value(), but that adds a lot of boilerplate code and it's not totally clear that rustc doesn't just optimize away the clone.

Well, as_tvm_value seems correct. I guess I'll just make the macros more macro-y to eliminate the repetitive code.

You could eliminate the allocation on the rust side doing something like:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=8d4048c7235d7e7bf8074f1514a07350

This wouldn't be able to go through Builder though.

Also, mapping a slice of tuples to a pair of slices is actually surprisingly hard to do with macros. You could avoid doing it if you're able to just call into() twice for each macro, or else you can do some advanced macro magic to map over the slice.

This can be addressed later.

ehsanmok · 2019-04-05T20:31:29Z

@nhynes @tqchen LGTM

kazimuth · 2019-04-05T22:06:33Z

e: see my review

tqchen · 2019-04-05T22:11:15Z

@kazimuth can you also https://docs.tvm.ai/contribute/code_review.html#approve-and-request-changes-explicitly

kazimuth · 2019-04-05T22:32:07Z

@tqchen sure

nhynes · 2019-04-05T23:11:23Z

I replaced into_tvm_value with to_tvm_value which takes &self. It copies the primitives and pointers, but this is actually correct since something needs to retain ownership of the strings and bytearrays.

ehsanmok · 2019-04-05T23:25:03Z

right! @nhynes if I remember correctly, into_tvm_value was necessary before esp. for box but it should be possible according to your changes now.

nhynes · 2019-04-05T23:55:13Z

It's no longer into because ffi::TVMValue doesn't own its contents. If, for instance, the frontend did into_tvm_value, there would be no guarantee that the pointed-to data wasn't dropped.

ehsanmok · 2019-04-07T01:06:47Z

@tqchen Could you merge this sooner please? I'm working on a feature which requires these fundamental changes and don't want to resolve too many conflicts.

tqchen · 2019-04-07T04:19:02Z

awaiting approval from @kazimuth

kazimuth · 2019-04-07T14:47:17Z

rust/frontend/src/function.rs

@@ -195,11 +195,11 @@ impl<'a, 'm> Builder<'a, 'm> {
        let (mut values, mut type_codes): (Vec<ffi::TVMValue>, Vec<ffi::TVMTypeCode>) = self
            .arg_buf
            .iter()
-            .map(|tvm_arg| (tvm_arg.value, tvm_arg.type_code as ffi::TVMTypeCode))
+            .map(|arg| arg.clone().into_tvm_value())


This can be addressed later.

kazimuth · 2019-04-07T14:48:02Z

Done @tqchen

tqchen · 2019-04-07T17:24:06Z

THanks, @nhynes , @kazimuth @ehsanmok , this is now merged

nhynes force-pushed the rust-next branch from 11feb29 to 0257ddc Compare April 5, 2019 04:45

TVMPODValue macro

b741271

nhynes force-pushed the rust-next branch from 0257ddc to b741271 Compare April 5, 2019 04:47

nhynes added 2 commits April 5, 2019 05:04

Update runtime to use new packedfunc

ab4f7dd

Update frontend

e7dc0f6

nhynes requested a review from tqchen April 5, 2019 06:44

nhynes marked this pull request as ready for review April 5, 2019 06:44

nhynes force-pushed the rust-next branch 3 times, most recently from 801da1f to 6698f72 Compare April 5, 2019 08:03

Update tests

0a9ccab

nhynes force-pushed the rust-next branch from 6698f72 to 0a9ccab Compare April 5, 2019 08:28

ehsanmok reviewed Apr 5, 2019

View reviewed changes

ehsanmok approved these changes Apr 5, 2019

View reviewed changes

to_tvm_value

d371edd

nhynes force-pushed the rust-next branch from 784081e to d371edd Compare April 5, 2019 23:22

nhynes mentioned this pull request Apr 6, 2019

[RUST] Rust DSO module #2976

Merged

kazimuth approved these changes Apr 7, 2019

View reviewed changes

tqchen merged commit 14a0ecb into apache:master Apr 7, 2019

tqchen added the status: accepted label Apr 7, 2019

wweic pushed a commit to wweic/tvm that referenced this pull request Apr 8, 2019

Rustify PackedFunc & Friends (apache#2969)

1e65e79

wweic pushed a commit to wweic/tvm that referenced this pull request Apr 10, 2019

Rustify PackedFunc & Friends (apache#2969)

048db2a

wweic pushed a commit to neo-ai/tvm that referenced this pull request Apr 11, 2019

Rustify PackedFunc & Friends (apache#2969)

6f719c6

tqchen mentioned this pull request Nov 8, 2019

[RELEASE][DRAFT] TVM v0.6 Release candidate #4259

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rustify PackedFunc & Friends #2969

Rustify PackedFunc & Friends #2969

nhynes commented Apr 5, 2019

nhynes commented Apr 5, 2019 •

edited

Loading

ehsanmok left a comment

ehsanmok Apr 5, 2019

ehsanmok Apr 5, 2019

nhynes Apr 5, 2019

nhynes Apr 5, 2019

kazimuth Apr 5, 2019

kazimuth Apr 7, 2019

ehsanmok commented Apr 5, 2019

kazimuth commented Apr 5, 2019 •

edited

Loading

tqchen commented Apr 5, 2019

kazimuth commented Apr 5, 2019 •

edited

Loading

nhynes commented Apr 5, 2019

ehsanmok commented Apr 5, 2019 •

edited

Loading

nhynes commented Apr 5, 2019

ehsanmok commented Apr 7, 2019 •

edited

Loading

tqchen commented Apr 7, 2019

kazimuth Apr 7, 2019

kazimuth commented Apr 7, 2019

tqchen commented Apr 7, 2019

Rustify PackedFunc & Friends #2969

Rustify PackedFunc & Friends #2969

Conversation

nhynes commented Apr 5, 2019

nhynes commented Apr 5, 2019 • edited Loading

ehsanmok left a comment

Choose a reason for hiding this comment

ehsanmok Apr 5, 2019

Choose a reason for hiding this comment

ehsanmok Apr 5, 2019

Choose a reason for hiding this comment

nhynes Apr 5, 2019

Choose a reason for hiding this comment

nhynes Apr 5, 2019

Choose a reason for hiding this comment

kazimuth Apr 5, 2019

Choose a reason for hiding this comment

kazimuth Apr 7, 2019

Choose a reason for hiding this comment

ehsanmok commented Apr 5, 2019

kazimuth commented Apr 5, 2019 • edited Loading

tqchen commented Apr 5, 2019

kazimuth commented Apr 5, 2019 • edited Loading

nhynes commented Apr 5, 2019

ehsanmok commented Apr 5, 2019 • edited Loading

nhynes commented Apr 5, 2019

ehsanmok commented Apr 7, 2019 • edited Loading

tqchen commented Apr 7, 2019

kazimuth Apr 7, 2019

Choose a reason for hiding this comment

kazimuth commented Apr 7, 2019

tqchen commented Apr 7, 2019

nhynes commented Apr 5, 2019 •

edited

Loading

kazimuth commented Apr 5, 2019 •

edited

Loading

kazimuth commented Apr 5, 2019 •

edited

Loading

ehsanmok commented Apr 5, 2019 •

edited

Loading

ehsanmok commented Apr 7, 2019 •

edited

Loading