Lunderberg commented on code in PR #16712:
URL: https://github.com/apache/tvm/pull/16712#discussion_r1545705155
##########
rust/tvm/src/ir/expr.rs:
##########
@@ -94,9 +94,9 @@ external! {
fn _as_text(object: ObjectRef, show_meta_data: i32, annotate:
runtime::Function) -> TString;
}
-pub fn as_text<T: IsObjectRef>(object: T) -> String {
+pub fn as_text<T: IsObjectRef>(object: T, show_meta_data: i32) -> String {
Review Comment:
1. Does the new argument break existing callsites of `as_text`? Since we
can't add default parameters to preserve backwards compatibility, I'm wondering
if this should be updated to use the builder pattern in the future.
2. Should the argument be `show_meta_data: bool` instead of `show_meta_data:
i32`? The FFI cannot currently distinguish between the two, but exposing it as
`i32` in the Rust bindings seems odd.
##########
rust/tvm/src/ir/attrs.rs:
##########
@@ -27,3 +27,19 @@ use tvm_macros::Object;
pub struct BaseAttrsNode {
pub base: Object,
}
+
+impl BaseAttrsNode {
+ pub fn base<T: IsObject>() -> BaseAttrsNode {
+ BaseAttrsNode {
+ base: Object::base::<T>(),
+ }
+ }
+}
+
+impl Attrs {
Review Comment:
On the C++ side, most `Attrs` are constructed using their subclass, not as a
`BaseAttrsNode`. Is this a difference that we want to have between the C++ and
Rust bindings?
##########
rust/tvm/src/ir/tir.rs:
##########
@@ -56,12 +56,35 @@ impl From<i32> for IntImm {
}
}
+impl From<i64> for IntImm {
+ fn from(i: i64) -> IntImm {
+ IntImm::new(DataType::int(64, 1), i)
+ }
+}
+
impl From<i32> for PrimExpr {
fn from(i: i32) -> PrimExpr {
IntImm::from(i).upcast()
}
}
+impl Into<i64> for PrimExpr {
Review Comment:
I don't think we should provide `Into<i64>` for `PrimExpr`, because `Into`
should only be used for infallible conversions. Can we provide `TryInto<i64>`
instead?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
