-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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][IRModule] Flesh out IRModule methods #6741
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The LLVM flags look good to me
rust/tvm-macros/src/external.rs
Outdated
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}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: line length
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WTB rustfmt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably just haven't run it yet, since everything is automatic I tend to just run it when I know I'm gtg.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: brief rustdoc? Some comments would help me get the story of what ExternalItem
and External
are for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What Greg said
let derives = quote! { | ||
impl std::hash::Hash for #ref_id { | ||
fn hash<H: std::hash::Hasher>(&self, state: &mut H) { | ||
self.0.hash(state) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we generate a wrapper new type over the ObjectPtr which contains 1 field always. ObjectPtr is currently dispatching to TVM's C++ hashing so we have cross language consistency.
@@ -83,6 +83,10 @@ impl DataType { | |||
DataType::new(DL_FLOAT_CODE, bits, lanes) | |||
} | |||
|
|||
pub const fn float32() -> DataType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is float32
, should the old float
be renamed to float64
(to avoid clashing with float
vs. double
size intuition?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use float/double terminology in TVM due to the fact that both integers and floating point values are both parametric by vector length and bit width. The float method here takes both bit width and vector length, unless I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple nit comments and questions. But it seems fine to merge, to me.
Haven't tested anything (I'm not up to speed yet on running TVM or using these rust bindings)
Its pretty easy to get started, @adelbertc has not repro'd running them if you are interested. I will finish adding tests and ping you guys again. |
9608878
to
d871c53
Compare
d871c53
to
726a01b
Compare
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What Greg said
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know rust very well, but looks good to me
* WIP * WIP * WIP * WIP * Disable WASM and fix rebase * Work on finishing tests * Make entire object system printable * Write some more tests for IRModule * All tests pass * Format * Restore module.cc * Bump syn
* WIP * WIP * WIP * WIP * Disable WASM and fix rebase * Work on finishing tests * Make entire object system printable * Write some more tests for IRModule * All tests pass * Format * Restore module.cc * Bump syn
* WIP * WIP * WIP * WIP * Disable WASM and fix rebase * Work on finishing tests * Make entire object system printable * Write some more tests for IRModule * All tests pass * Format * Restore module.cc * Bump syn
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