You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/10/23 06:22:27 UTC
[GitHub] [incubator-tvm] jroesch opened a new pull request #6741: [Rust][IRModule] Flesh out IRModule methods
jroesch opened a new pull request #6741:
URL: https://github.com/apache/incubator-tvm/pull/6741
This PR fully expand the IRModule interface in Rust and fixes a whole host of sharp edges and improves the procedural macros for generating the boilerplate.
cc @mwillsey @altanh @imalsogreg @gussmith23
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] imalsogreg commented on a change in pull request #6741: [Rust][IRModule] Flesh out IRModule methods
Posted by GitBox <gi...@apache.org>.
imalsogreg commented on a change in pull request #6741:
URL: https://github.com/apache/incubator-tvm/pull/6741#discussion_r512015019
##########
File path: rust/tvm/src/ir/module.rs
##########
@@ -156,4 +145,127 @@ impl IRModule {
{
module_lookup_str(self.clone(), name.into())
}
+
+ pub fn get_global_type_vars(&self) -> Result<Array<GlobalTypeVar>> {
+ module_get_global_type_vars(self.clone())
Review comment:
Is `clone()` on an `IRModule` expensive?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] imalsogreg commented on a change in pull request #6741: [Rust][IRModule] Flesh out IRModule methods
Posted by GitBox <gi...@apache.org>.
imalsogreg commented on a change in pull request #6741:
URL: https://github.com/apache/incubator-tvm/pull/6741#discussion_r511993471
##########
File path: rust/tvm-macros/src/external.rs
##########
@@ -17,12 +17,32 @@
* under the License.
*/
use proc_macro2::Span;
+use proc_macro_error::abort;
use quote::quote;
use syn::parse::{Parse, ParseStream, Result};
-use syn::{FnArg, Generics, Ident, Lit, Meta, NestedMeta, Pat, ReturnType, TraitItemMethod, Type};
+use syn::{FnArg, Signature, Attribute, token::Semi, Visibility, Generics, Ident, Lit, Meta, NestedMeta, Pat, ReturnType, Type};
Review comment:
nit: line length
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6741: [Rust][IRModule] Flesh out IRModule methods
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6741:
URL: https://github.com/apache/incubator-tvm/pull/6741#discussion_r517766695
##########
File path: rust/tvm-macros/src/object.rs
##########
@@ -185,5 +187,26 @@ pub fn macro_impl(input: proc_macro::TokenStream) -> TokenStream {
expanded.extend(base_tokens);
+ if derive {
+ let derives = quote! {
+ impl std::hash::Hash for #ref_id {
+ fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+ self.0.hash(state)
Review comment:
No we generate a wrapper new type over the ObjectPtr<T> which contains 1 field always. ObjectPtr is currently dispatching to TVM's C++ hashing so we have cross language consistency.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on pull request #6741: [Rust][IRModule] Flesh out IRModule methods
Posted by GitBox <gi...@apache.org>.
jroesch commented on pull request #6741:
URL: https://github.com/apache/incubator-tvm/pull/6741#issuecomment-722196000
@adelbertc @imalsogreg I will try and ship docs for proc macro after landing this branch if that's cool with you guys, just so I can unblock some other branches I have.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] adelbertc commented on a change in pull request #6741: [Rust][IRModule] Flesh out IRModule methods
Posted by GitBox <gi...@apache.org>.
adelbertc commented on a change in pull request #6741:
URL: https://github.com/apache/incubator-tvm/pull/6741#discussion_r517787579
##########
File path: rust/tvm-macros/src/external.rs
##########
@@ -124,15 +151,21 @@ pub fn macro_impl(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
let ty: Type = *pat_type.ty.clone();
(ident, ty)
}
- _ => panic!(),
+ _ => abort! { pat_type,
Review comment:
Is `abort!` the standard way to blow up a macro?
##########
File path: rust/tvm-macros/src/object.rs
##########
@@ -75,6 +79,12 @@ pub fn macro_impl(input: proc_macro::TokenStream) -> TokenStream {
_ => panic!("derive only works for structs"),
};
+ let ref_derives = if derive {
Review comment:
This is like if you mark something with a `no_derive` attribute you don't derive `Debug`? Naming is a bit wonky here
##########
File path: rust/tvm-rt/src/array.rs
##########
@@ -82,6 +82,13 @@ impl<T: IsObjectRef> Array<T> {
}
}
+impl<T: IsObjectRef> std::fmt::Debug for Array<T> {
+ fn fmt(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
+ let as_vec: Vec<T> = self.clone().into_iter().collect();
Review comment:
Instead of consuming `self` with `into_iter` could you have a method that returns an `Iterator` over references so you don't have to clone, since `write!` I assume doesn't need owned values? But this may not matter if `clone`ing these `Array`s isn't expected to be expensive.
##########
File path: rust/tvm-macros/src/external.rs
##########
@@ -17,12 +17,32 @@
* under the License.
*/
use proc_macro2::Span;
+use proc_macro_error::abort;
use quote::quote;
use syn::parse::{Parse, ParseStream, Result};
-use syn::{FnArg, Generics, Ident, Lit, Meta, NestedMeta, Pat, ReturnType, TraitItemMethod, Type};
+use syn::{FnArg, Signature, Attribute, token::Semi, Visibility, Generics, Ident, Lit, Meta, NestedMeta, Pat, ReturnType, Type};
+
+struct ExternalItem {
Review comment:
What Greg said
##########
File path: rust/tvm-rt/src/object/object_ptr.rs
##########
@@ -147,14 +148,26 @@ impl Object {
}
}
+// impl fmt::Debug for Object {
Review comment:
remove dead code
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6741: [Rust][IRModule] Flesh out IRModule methods
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6741:
URL: https://github.com/apache/incubator-tvm/pull/6741#discussion_r517789055
##########
File path: rust/tvm-macros/src/object.rs
##########
@@ -75,6 +79,12 @@ pub fn macro_impl(input: proc_macro::TokenStream) -> TokenStream {
_ => panic!("derive only works for structs"),
};
+ let ref_derives = if derive {
Review comment:
yeah currently I just turn off all deriving for types marked no-derive (this all to support String which is like complete bespoke impl)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6741: [Rust][IRModule] Flesh out IRModule methods
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6741:
URL: https://github.com/apache/incubator-tvm/pull/6741#discussion_r517788886
##########
File path: rust/tvm-macros/src/external.rs
##########
@@ -124,15 +151,21 @@ pub fn macro_impl(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
let ty: Type = *pat_type.ty.clone();
(ident, ty)
}
- _ => panic!(),
+ _ => abort! { pat_type,
Review comment:
This is proc_macro_error which is crate for doing better errors in proc macros.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch merged pull request #6741: [Rust][IRModule] Flesh out IRModule methods
Posted by GitBox <gi...@apache.org>.
jroesch merged pull request #6741:
URL: https://github.com/apache/incubator-tvm/pull/6741
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6741: [Rust][IRModule] Flesh out IRModule methods
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6741:
URL: https://github.com/apache/incubator-tvm/pull/6741#discussion_r517838659
##########
File path: rust/tvm-rt/src/object/object_ptr.rs
##########
@@ -147,14 +148,26 @@ impl Object {
}
}
+// impl fmt::Debug for Object {
Review comment:
I was leaving this here for after I land another outstanding branch so I can clean up some of the code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] imalsogreg commented on a change in pull request #6741: [Rust][IRModule] Flesh out IRModule methods
Posted by GitBox <gi...@apache.org>.
imalsogreg commented on a change in pull request #6741:
URL: https://github.com/apache/incubator-tvm/pull/6741#discussion_r512001076
##########
File path: rust/tvm-rt/src/value.rs
##########
@@ -22,7 +22,6 @@
//! `RetValue` is the owned version of `TVMPODValue`.
use std::convert::TryFrom;
-// use std::ffi::c_void;
Review comment:
stray comment
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] imalsogreg commented on a change in pull request #6741: [Rust][IRModule] Flesh out IRModule methods
Posted by GitBox <gi...@apache.org>.
imalsogreg commented on a change in pull request #6741:
URL: https://github.com/apache/incubator-tvm/pull/6741#discussion_r512007489
##########
File path: rust/tvm-macros/src/object.rs
##########
@@ -185,5 +187,26 @@ pub fn macro_impl(input: proc_macro::TokenStream) -> TokenStream {
expanded.extend(base_tokens);
+ if derive {
+ let derives = quote! {
+ impl std::hash::Hash for #ref_id {
+ fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+ self.0.hash(state)
Review comment:
Possibly dumb comment (I'm not familiar with this macro) - is `self.0` always the right thing to hash? Or is `#ref_id` sometimes a tuple with multiple fields, where field `.1` and `.2` etc. may be part of what distinguishes objects when hashing?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] jroesch commented on a change in pull request #6741: [Rust][IRModule] Flesh out IRModule methods
Posted by GitBox <gi...@apache.org>.
jroesch commented on a change in pull request #6741:
URL: https://github.com/apache/incubator-tvm/pull/6741#discussion_r517789274
##########
File path: rust/tvm-rt/src/array.rs
##########
@@ -82,6 +82,13 @@ impl<T: IsObjectRef> Array<T> {
}
}
+impl<T: IsObjectRef> std::fmt::Debug for Array<T> {
+ fn fmt(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
+ let as_vec: Vec<T> = self.clone().into_iter().collect();
Review comment:
its just cheaper to clone the outer smart pointer because we only modify one reference count instead of touching the ref count on every single element.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] imalsogreg commented on a change in pull request #6741: [Rust][IRModule] Flesh out IRModule methods
Posted by GitBox <gi...@apache.org>.
imalsogreg commented on a change in pull request #6741:
URL: https://github.com/apache/incubator-tvm/pull/6741#discussion_r512003693
##########
File path: rust/tvm-macros/src/external.rs
##########
@@ -17,12 +17,32 @@
* under the License.
*/
use proc_macro2::Span;
+use proc_macro_error::abort;
use quote::quote;
use syn::parse::{Parse, ParseStream, Result};
-use syn::{FnArg, Generics, Ident, Lit, Meta, NestedMeta, Pat, ReturnType, TraitItemMethod, Type};
+use syn::{FnArg, Signature, Attribute, token::Semi, Visibility, Generics, Ident, Lit, Meta, NestedMeta, Pat, ReturnType, Type};
+
+struct ExternalItem {
Review comment:
nit: brief rustdoc? Some comments would help me get the story of what `ExternalItem` and `External` are for.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] imalsogreg commented on a change in pull request #6741: [Rust][IRModule] Flesh out IRModule methods
Posted by GitBox <gi...@apache.org>.
imalsogreg commented on a change in pull request #6741:
URL: https://github.com/apache/incubator-tvm/pull/6741#discussion_r512011355
##########
File path: rust/tvm-sys/src/datatype.rs
##########
@@ -83,6 +83,10 @@ impl DataType {
DataType::new(DL_FLOAT_CODE, bits, lanes)
}
+ pub const fn float32() -> DataType {
Review comment:
if this is `float32`, should the old `float` be renamed to `float64` (to avoid clashing with `float` vs. `double` size intuition?)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org