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/06/12 15:16:38 UTC
[GitHub] [incubator-tvm] ANSHUMAN87 opened a new pull request #5788: Migrate IntImm & FloatImm ObjectRef to not-null
ANSHUMAN87 opened a new pull request #5788:
URL: https://github.com/apache/incubator-tvm/pull/5788
Refer #5318
@tqchen, @jroesch, @zhiics, @junrushao1994 : Please help review, Thanks!
----------------------------------------------------------------
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] tqchen commented on a change in pull request #5788: Migrate IntImm & FloatImm ObjectRef to not-null
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5788:
URL: https://github.com/apache/incubator-tvm/pull/5788#discussion_r439850074
##########
File path: src/arith/rewrite_simplify.cc
##########
@@ -123,7 +123,7 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const AddNode* op) {
// Pattern var to match any expression
PVar<PrimExpr> x, y, z, b1, b2, s1, s2;
// Pattern var match IntImm
- PVar<IntImm> c1, c2, c3;
+ PVarOpt<Optional<IntImm>> c1, c2, c3;
Review comment:
I don't think either approaches are good here. Let us leave it as it is for now
----------------------------------------------------------------
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] ANSHUMAN87 commented on a change in pull request #5788: Migrate IntImm & FloatImm ObjectRef to not-null
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on a change in pull request #5788:
URL: https://github.com/apache/incubator-tvm/pull/5788#discussion_r439848026
##########
File path: src/arith/rewrite_simplify.cc
##########
@@ -123,7 +123,7 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const AddNode* op) {
// Pattern var to match any expression
PVar<PrimExpr> x, y, z, b1, b2, s1, s2;
// Pattern var match IntImm
- PVar<IntImm> c1, c2, c3;
+ PVarOpt<Optional<IntImm>> c1, c2, c3;
Review comment:
I have 2 possible approach here:
A0: Rename PVarOpt --> PVarExt(Extended) and hide Optional<T> input format.
Look like --> `PVarExt<IntImm> c1, c2, c3;`
A1: Provide default constructor with default values.
Look like: `PVar<IntImm> c1(IntImm(DataType::Int(32), 0)), c2(IntImm(DataType::Int(32), 0)), c3(IntImm(DataType::Int(32), 0));`
Please let me know your thoughts on this. Thanks!
----------------------------------------------------------------
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] ANSHUMAN87 commented on a change in pull request #5788: Migrate IntImm & FloatImm ObjectRef to not-null
Posted by GitBox <gi...@apache.org>.
ANSHUMAN87 commented on a change in pull request #5788:
URL: https://github.com/apache/incubator-tvm/pull/5788#discussion_r439498500
##########
File path: src/arith/rewrite_simplify.cc
##########
@@ -123,7 +123,7 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const AddNode* op) {
// Pattern var to match any expression
PVar<PrimExpr> x, y, z, b1, b2, s1, s2;
// Pattern var match IntImm
- PVar<IntImm> c1, c2, c3;
+ PVarOpt<Optional<IntImm>> c1, c2, c3;
Review comment:
Yes, I agree. It is only to accommodate absent of default constructor. However the behaviour don't violate with PVar I think.
----------------------------------------------------------------
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] junrushao1994 commented on a change in pull request #5788: Migrate IntImm & FloatImm ObjectRef to not-null
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on a change in pull request #5788:
URL: https://github.com/apache/incubator-tvm/pull/5788#discussion_r439553237
##########
File path: src/arith/rewrite_simplify.cc
##########
@@ -123,7 +123,7 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const AddNode* op) {
// Pattern var to match any expression
PVar<PrimExpr> x, y, z, b1, b2, s1, s2;
// Pattern var match IntImm
- PVar<IntImm> c1, c2, c3;
+ PVarOpt<Optional<IntImm>> c1, c2, c3;
Review comment:
I agree too...Is there any better to do this?
----------------------------------------------------------------
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] tqchen commented on a change in pull request #5788: Migrate IntImm & FloatImm ObjectRef to not-null
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5788:
URL: https://github.com/apache/incubator-tvm/pull/5788#discussion_r439850074
##########
File path: src/arith/rewrite_simplify.cc
##########
@@ -123,7 +123,7 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const AddNode* op) {
// Pattern var to match any expression
PVar<PrimExpr> x, y, z, b1, b2, s1, s2;
// Pattern var match IntImm
- PVar<IntImm> c1, c2, c3;
+ PVarOpt<Optional<IntImm>> c1, c2, c3;
Review comment:
I don;t think either approaches are good here. Let us leave it as it is for now
----------------------------------------------------------------
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] tqchen closed pull request #5788: Migrate IntImm & FloatImm ObjectRef to not-null
Posted by GitBox <gi...@apache.org>.
tqchen closed pull request #5788:
URL: https://github.com/apache/incubator-tvm/pull/5788
----------------------------------------------------------------
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] tqchen commented on a change in pull request #5788: Migrate IntImm & FloatImm ObjectRef to not-null
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #5788:
URL: https://github.com/apache/incubator-tvm/pull/5788#discussion_r439487451
##########
File path: src/arith/rewrite_simplify.cc
##########
@@ -123,7 +123,7 @@ PrimExpr RewriteSimplifier::Impl::VisitExpr_(const AddNode* op) {
// Pattern var to match any expression
PVar<PrimExpr> x, y, z, b1, b2, s1, s2;
// Pattern var match IntImm
- PVar<IntImm> c1, c2, c3;
+ PVarOpt<Optional<IntImm>> c1, c2, c3;
Review comment:
I don't think it is right to use optional here, as the intention is to match the variable itself
----------------------------------------------------------------
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