[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-24 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r414946295



##
File path: src/te/operation/compute_op.cc
##
@@ -231,20 +231,18 @@ void ComputeOpNode::PropBoundToInputs(
   // undefined behaviour), so we can intersect the estimated set of 
the argument with the
   // range expected by the tensor. However, intersection may result in 
overly complex
   // expressions, so we perform a more relaxed form of intersection.
-  IntSet arg_intset = EvalSet(call->args[i], dom_map);
+  IntSet arg_intset = analyzer->int_set(call->args[i], 
ConvertDomMap(dom_map));
   const arith::IntervalSetNode* arg_interval = 
arg_intset.as();
   if (arg_interval) {
 PrimExpr shape_i_min_value = make_zero(t->shape[i].dtype());
 PrimExpr shape_i_max_value = t->shape[i] - 1;
 PrimExpr min_value = arg_interval->min_value;
 PrimExpr max_value = arg_interval->max_value;
 // Prefer the shape bounds only when we can prove they are tighter.
-if (arith::is_neg_inf(min_value) ||
-analyzer->CanProve(shape_i_min_value >= min_value)) {
+if ((arith::is_pos_inf(max_value) && arith::is_neg_inf(min_value)) 
||
+(analyzer->CanProve(shape_i_min_value >= min_value) &&
+ analyzer->CanProve(shape_i_max_value <= max_value))) {

Review comment:
   `tests/python/unittest/test_target_codegen_blob.py` exposes a case, 
where `shape_i` is `[0, 0]` and arg_interval is `[threadIdx.y, 
threadIdx.y]`,where `threadIdx.y`'s range is `[0, 7]`.  Either `[0, 0]` or 
`[threadIdx.y, threadIdx.y]` is fine.  The current logic ends with 
`[threadIdx.y, 0]`, messing up further transforms and triggering an assertion 
later.  We'd better to update both ends at the same time, i.e. treating 
IntervalSet as an atomic object.  Adding this explanation as comment in 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:
us...@infra.apache.org




[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-24 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r414944483



##
File path: include/tvm/arith/analyzer.h
##
@@ -411,8 +412,9 @@ class TVM_DLL Analyzer {
*
* \param var The variable.
* \param expr The expression we bind to.
+   * \param override Whether do we allow override of existing information.

Review comment:
   Rephrasing the comments.  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:
us...@infra.apache.org




[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-24 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r414940958



##
File path: src/arith/int_set.cc
##
@@ -311,6 +311,16 @@ inline IntervalSet Combine(Analyzer* 
analyzer,
   LOG(FATAL) << "Modular by zero in CombineInterval Mod";
 }
 if (analyzer->CanProveGreaterEqual(divisor, 0)) {
+  if (b->min_value.as()) {

Review comment:
   Make sense.  I will make the change.





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:
us...@infra.apache.org




[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-24 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r414674995



##
File path: src/te/operation/compute_op.cc
##
@@ -231,20 +231,18 @@ void ComputeOpNode::PropBoundToInputs(
   // undefined behaviour), so we can intersect the estimated set of 
the argument with the
   // range expected by the tensor. However, intersection may result in 
overly complex
   // expressions, so we perform a more relaxed form of intersection.
-  IntSet arg_intset = EvalSet(call->args[i], dom_map);
+  IntSet arg_intset = analyzer->int_set(call->args[i], 
ConvertDomMap(dom_map));
   const arith::IntervalSetNode* arg_interval = 
arg_intset.as();
   if (arg_interval) {
 PrimExpr shape_i_min_value = make_zero(t->shape[i].dtype());
 PrimExpr shape_i_max_value = t->shape[i] - 1;
 PrimExpr min_value = arg_interval->min_value;
 PrimExpr max_value = arg_interval->max_value;
 // Prefer the shape bounds only when we can prove they are tighter.
-if (arith::is_neg_inf(min_value) ||
-analyzer->CanProve(shape_i_min_value >= min_value)) {
+if (arith::is_pos_inf(max_value) || arith::is_neg_inf(min_value) ||

Review comment:
   No, I only mean to change a bound's ends in pair.  Correcting the code.  
Thank you for pointing it out.





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:
us...@infra.apache.org




[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-24 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r414674995



##
File path: src/te/operation/compute_op.cc
##
@@ -231,20 +231,18 @@ void ComputeOpNode::PropBoundToInputs(
   // undefined behaviour), so we can intersect the estimated set of 
the argument with the
   // range expected by the tensor. However, intersection may result in 
overly complex
   // expressions, so we perform a more relaxed form of intersection.
-  IntSet arg_intset = EvalSet(call->args[i], dom_map);
+  IntSet arg_intset = analyzer->int_set(call->args[i], 
ConvertDomMap(dom_map));
   const arith::IntervalSetNode* arg_interval = 
arg_intset.as();
   if (arg_interval) {
 PrimExpr shape_i_min_value = make_zero(t->shape[i].dtype());
 PrimExpr shape_i_max_value = t->shape[i] - 1;
 PrimExpr min_value = arg_interval->min_value;
 PrimExpr max_value = arg_interval->max_value;
 // Prefer the shape bounds only when we can prove they are tighter.
-if (arith::is_neg_inf(min_value) ||
-analyzer->CanProve(shape_i_min_value >= min_value)) {
+if (arith::is_pos_inf(max_value) || arith::is_neg_inf(min_value) ||

Review comment:
   No, I only mean to change a bound's ends in pair.  Correcting the code.  
Thank you for point it out.





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:
us...@infra.apache.org




[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-23 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r414263234



##
File path: src/arith/int_set.cc
##
@@ -311,6 +311,21 @@ inline IntervalSet Combine(Analyzer* 
analyzer,
   LOG(FATAL) << "Modular by zero in CombineInterval Mod";
 }
 if (analyzer->CanProveGreaterEqual(divisor, 0)) {
+  if (const auto* ptr = b->min_value.as()) {
+// a mod b = a - b * (a/b) if
+// (i) a_max - a_min < b, i.e. that before mod, a's range doesn't 
cover [0, b)
+// and (ii) a_min mod b <= a_max mod b, i.e. that a's range is still 
continuous after mod
+auto tmax = a->max_value - b->min_value * floordiv(a->max_value, 
b->min_value);
+tmax = analyzer->Simplify(tmax);
+auto tmin = a->min_value - b->min_value * floordiv(a->min_value, 
b->min_value);
+tmin = analyzer->Simplify(tmin);
+auto tset = IntervalSet(tmin, tmax);
+bool within_range = analyzer->CanProveLess(a->max_value - 
a->min_value, ptr->value);
+bool wrap_around = analyzer->CanProve(tset->max_value < 
tset->min_value);

Review comment:
   I missed the point that CanProve is necessary but not sufficient here.  
Once I modified the condition to tset->max_value >= tset->min_value, 
floormod([z*8+x*4, z*8+x*4+3], 8) fell back to [0, 7], matching your earlier 
suggested floormod implementation, because the it can't prove (((x*4) + 3) - 
(floordiv(((x*4) + 3), 8)*8)) >= ((x*4) - (floordiv(x, 2)*8)).  Therefore, I 
switch to the simple implementation and modify the test.

##
File path: src/arith/int_set.cc
##
@@ -311,6 +311,21 @@ inline IntervalSet Combine(Analyzer* 
analyzer,
   LOG(FATAL) << "Modular by zero in CombineInterval Mod";
 }
 if (analyzer->CanProveGreaterEqual(divisor, 0)) {
+  if (const auto* ptr = b->min_value.as()) {
+// a mod b = a - b * (a/b) if
+// (i) a_max - a_min < b, i.e. that before mod, a's range doesn't 
cover [0, b)
+// and (ii) a_min mod b <= a_max mod b, i.e. that a's range is still 
continuous after mod
+auto tmax = a->max_value - b->min_value * floordiv(a->max_value, 
b->min_value);
+tmax = analyzer->Simplify(tmax);
+auto tmin = a->min_value - b->min_value * floordiv(a->min_value, 
b->min_value);
+tmin = analyzer->Simplify(tmin);
+auto tset = IntervalSet(tmin, tmax);
+bool within_range = analyzer->CanProveLess(a->max_value - 
a->min_value, ptr->value);
+bool wrap_around = analyzer->CanProve(tset->max_value < 
tset->min_value);
+if (within_range && !wrap_around) {
+  return tset;

Review comment:
   No longer an issue with the update.





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:
us...@infra.apache.org




[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-23 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r414259311



##
File path: include/tvm/arith/int_set.h
##
@@ -152,6 +152,22 @@ class IntSet : public ObjectRef {
 //---
 // Integer set legacy API.
 //
+/*!
+ * \brief Convert std::unordered_map to Map
+ *
+ * \param dom_map The domain map to convert.
+ * \return The converted map.
+ */
+Map ConvertDomMap(const std::unordered_map& dom_map);
+// /*!
+//  * \brief Find an symbolic integer set that contains all possible values of
+//  *  e given the domain of each iteration variables.
+//  *
+//  * \param e The expression to be evaluated.
+//  * \param dom_map The domain of each variable.
+//  * \return An integer set that can cover all the possible values of e.
+//  */
+// IntSet EvalSet(PrimExpr e, const Map& dom_map);

Review comment:
   My bad.  Cleaned it up.





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:
us...@infra.apache.org




[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-23 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r414259311



##
File path: include/tvm/arith/int_set.h
##
@@ -152,6 +152,22 @@ class IntSet : public ObjectRef {
 //---
 // Integer set legacy API.
 //
+/*!
+ * \brief Convert std::unordered_map to Map
+ *
+ * \param dom_map The domain map to convert.
+ * \return The converted map.
+ */
+Map ConvertDomMap(const std::unordered_map& dom_map);
+// /*!
+//  * \brief Find an symbolic integer set that contains all possible values of
+//  *  e given the domain of each iteration variables.
+//  *
+//  * \param e The expression to be evaluated.
+//  * \param dom_map The domain of each variable.
+//  * \return An integer set that can cover all the possible values of e.
+//  */
+// IntSet EvalSet(PrimExpr e, const Map& dom_map);

Review comment:
   My bad.  Clean it up.





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:
us...@infra.apache.org




[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-23 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r414034656



##
File path: src/arith/const_int_bound.cc
##
@@ -150,10 +150,12 @@ class ConstIntBoundAnalyzer::Impl :
   const PrimExprNode* op = expr.as();
   auto val = bound_->find(op);
   if (val != bound_->end()) {
-CHECK(val->second->min_value == res.min_value &&
-  val->second->max_value == res.max_value)
-  << "Detected bound for " << expr
-  << "conflicts with memorization";
+auto everything = Everything(op->dtype);
+CHECK(
+(val->second->min_value == res.min_value && val->second->max_value 
== res.max_value) ||
+(val->second->min_value == everything.min_value &&

Review comment:
   @hzfan do you mean to update val->second, when res is a subset?  It is a 
looser check than the current change.  I haven't seen any case that a limit 
bound improves from a looser limit bound.  If we don't have any such case now, 
I'd like to let this check assert for people to justify when they change the 
behavior. 





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:
us...@infra.apache.org




[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-23 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r413843881



##
File path: src/arith/int_set.cc
##
@@ -518,7 +533,14 @@ class IntervalSetEvaluator :
   // whether set is exactly single point that equals value.
   bool MatchPoint(const IntervalSet& set,
   const PrimExpr& value) const {
-return set->min_value.same_as(value) && set->max_value.same_as(value);
+if (set->min_value.same_as(value) && set->max_value.same_as(value)) {

Review comment:
   I don't need to change MatchPoint and IsSinglePoint after binding all 
ranges to one analyzer.  I have reverted the changes to these two functions.





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:
us...@infra.apache.org




[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-23 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r413843721



##
File path: src/arith/int_set.cc
##
@@ -372,7 +387,27 @@ class IntervalSetEvaluator :
   }
 
   IntervalSet Eval(const PrimExpr& val) {
-return this->VisitExpr(val);
+IntervalSet result = this->VisitExpr(val);

Review comment:
   This change is no longer needed.





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:
us...@infra.apache.org




[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-22 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r413285295



##
File path: src/te/schedule/bound.cc
##
@@ -199,11 +202,13 @@ void InferRootBound(const Stage& stage,
 r = iv->dom;
   }
   if (relax_set.size() != 0) {
-dom_map[iv->var.get()] = EvalSet(r, relax_set);
+dom_map[iv->var.get()] = IntSet::interval(
+analyzer.int_set(r->min, relax_set).min(),
+analyzer.int_set(r->min + r->extent - 1, relax_set).max());
   } else {
 dom_map[iv->var.get()] = IntSet::range(r);
   }
-  analyzer.Bind(iv->var, r);
+  analyzer.Bind(iv->var, r, true);

Review comment:
   The previous Bind call binds root IterVars.  We have to override them 
here.  Is there a easy way to tell an IterVar root IterVar?  If so, we can 
avoid such binding/overriding.





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:
us...@infra.apache.org




[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-22 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r413277656



##
File path: src/arith/int_set.cc
##
@@ -372,7 +387,27 @@ class IntervalSetEvaluator :
   }
 
   IntervalSet Eval(const PrimExpr& val) {
-return this->VisitExpr(val);
+IntervalSet result = this->VisitExpr(val);
+// Use the IterVar range info bound to analyzer to further simplify
+// and reduce the interval
+auto min_value_expr = analyzer_->Simplify(result->min_value);
+auto max_value_expr = analyzer_->Simplify(result->max_value);
+auto min_bd = analyzer_->const_int_bound(min_value_expr);
+auto max_bd = analyzer_->const_int_bound(max_value_expr);
+if (min_bd->max_value == min_bd->min_value && max_bd->max_value == 
max_bd->min_value) {
+  const auto* min_ptr = result->min_value.as();
+  const auto* max_ptr = result->max_value.as();
+  // The following if statement is necessary.  When result is a single 
point of IntImm, such as
+  // [0, 0], both 0s refer the same ObjectRef.  We really don't want to 
create a new [0, 0]
+  // IntervalSet and have 0s refer two different ObjectRef.  They will 
confuse APIs, such as
+  // IntervalSetEvaluator::MatchPoint() and 
IntervalSetNode::IsSinglePoint().
+  if (min_ptr && max_ptr && min_bd->min_value == min_ptr->value &&

Review comment:
   Moved the IntImm handling to MatchPoint and 
IntervalSetNode::IsSinglePoint().  Reverted changes here.





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:
us...@infra.apache.org




[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-22 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r413205094



##
File path: src/arith/int_set.cc
##
@@ -518,7 +533,14 @@ class IntervalSetEvaluator :
   // whether set is exactly single point that equals value.
   bool MatchPoint(const IntervalSet& set,
   const PrimExpr& value) const {
-return set->min_value.same_as(value) && set->max_value.same_as(value);
+if (set->min_value.same_as(value) && set->max_value.same_as(value)) {

Review comment:
   Make sense.  I will keep the .same_as(value) calls for performance and 
use CanProve for the additional comparison.





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:
us...@infra.apache.org




[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-22 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r413188821



##
File path: src/arith/const_int_bound.cc
##
@@ -150,10 +150,12 @@ class ConstIntBoundAnalyzer::Impl :
   const PrimExprNode* op = expr.as();
   auto val = bound_->find(op);
   if (val != bound_->end()) {
-CHECK(val->second->min_value == res.min_value &&
-  val->second->max_value == res.max_value)
-  << "Detected bound for " << expr
-  << "conflicts with memorization";
+auto everything = Everything(op->dtype);
+CHECK(
+(val->second->min_value == res.min_value && val->second->max_value 
== res.max_value) ||
+(val->second->min_value == everything.min_value &&

Review comment:
   It is not exactly override, but assuming a partial order that limit 
bound is better than inf bound.  I still keep assertion for different limit 
bounds, when override == false.
   This change alters API semantics, but I am not sure if this behavior worth 
another mode in addition to override.





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:
us...@infra.apache.org




[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-22 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r413183510



##
File path: src/arith/const_int_bound.cc
##
@@ -150,10 +150,12 @@ class ConstIntBoundAnalyzer::Impl :
   const PrimExprNode* op = expr.as();
   auto val = bound_->find(op);
   if (val != bound_->end()) {
-CHECK(val->second->min_value == res.min_value &&
-  val->second->max_value == res.max_value)
-  << "Detected bound for " << expr
-  << "conflicts with memorization";
+auto everything = Everything(op->dtype);
+CHECK(
+(val->second->min_value == res.min_value && val->second->max_value 
== res.max_value) ||
+(val->second->min_value == everything.min_value &&

Review comment:
   Good catch.  I forgot to bring this up for discussion.





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:
us...@infra.apache.org




[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-20 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r411886246



##
File path: include/tvm/te/operation.h
##
@@ -112,6 +113,7 @@ class OperationNode : public tir::FunctionBaseNode {
   const Operation& self,
   arith::Analyzer* analyzer,
   const std::unordered_map& dom_map,
+  const std::unordered_map& rmap,

Review comment:
   Don't need this change as no need to pass rmap to EvalSet any more.





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:
us...@infra.apache.org




[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-20 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r411885530



##
File path: src/arith/int_set.cc
##
@@ -730,12 +765,30 @@ Map ConvertDomMap(
   return dmap;
 }
 
-IntSet EvalSet(PrimExpr e,
-   const Map& dom_map) {
+IntSet EvalSet(PrimExpr e, const Map& dom_map,
+   const std::unordered_map& rmap) {
   Analyzer ana;
+  // Bind ana with rmap
+  for (auto entry : rmap) {
+ana.Bind(entry.first->var, entry.second);

Review comment:
   Don't need this change any more.





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:
us...@infra.apache.org




[GitHub] [incubator-tvm] yongfeng-nv commented on a change in pull request #5367: Improve IntervalSet's floormod

2020-04-20 Thread GitBox


yongfeng-nv commented on a change in pull request #5367:
URL: https://github.com/apache/incubator-tvm/pull/5367#discussion_r411785613



##
File path: src/arith/int_set.cc
##
@@ -372,7 +387,27 @@ class IntervalSetEvaluator :
   }
 
   IntervalSet Eval(const PrimExpr& val) {
-return this->VisitExpr(val);
+IntervalSet result = this->VisitExpr(val);
+// Use the IterVar range info bound to analyzer to further simplify
+// and reduce the interval
+auto min_value_expr = analyzer_->Simplify(result->min_value);
+auto max_value_expr = analyzer_->Simplify(result->max_value);
+auto min_bd = analyzer_->const_int_bound(min_value_expr);
+auto max_bd = analyzer_->const_int_bound(max_value_expr);
+if (min_bd->max_value == min_bd->min_value && max_bd->max_value == 
max_bd->min_value) {
+  const auto* min_ptr = result->min_value.as();
+  const auto* max_ptr = result->max_value.as();
+  // The following if statement is necessary.  When result is a single 
point of IntImm, such as
+  // [0, 0], both 0s refer the same ObjectRef.  We really don't want to 
create a new [0, 0]
+  // IntervalSet and have 0s refer two different ObjectRef.  They will 
confuse APIs, such as
+  // IntervalSetEvaluator::MatchPoint() and 
IntervalSetNode::IsSinglePoint().
+  if (min_ptr && max_ptr && min_bd->min_value == min_ptr->value &&

Review comment:
   I tried to fix MatchPoint first, then I found out 
IntervalSetNode::IsSinglePoint() requiring update too and worried about more 
APIs to touch.  Now I am moving the IntImm single point handling to IntervalSet 
constructor.  At certain point, I think immediate values are better to refactor 
to singleton.
   





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:
us...@infra.apache.org