[ARITH][IR] Introduce FloorDiv/Mod #3479

tqchen · 2019-07-03T00:28:12Z

Code acompany #3478

tqchen · 2019-07-04T19:53:25Z

cc @derisavi @sgrechanik-h @yzhliu @merrymercy @Hzfengsy @kazum @ZihengJiang

sgrechanik-h · 2019-07-05T05:56:26Z

src/arithmetic/canonical_simplify.cc

@@ -63,6 +64,29 @@ class CanonicalExprNode : public BaseExprNode {
  TVM_DECLARE_BASE_NODE_INFO(CanonicalExprNode, BaseExprNode);
 };

+enum DivMode {
+  /*! \brief Trucated division. */
+  kTrucDiv,


Typo Truc -> Trunc

sgrechanik-h · 2019-07-05T05:57:26Z

src/arithmetic/canonical_simplify.cc

+inline Expr ModImpl(Expr a, Expr b, DivMode mode) {
+  if (mode == kTrucDiv) {
+    return a % b;
+  } else {


Maybe explicitly check mode == kFloorDiv

sgrechanik-h · 2019-07-05T07:33:46Z

src/arithmetic/canonical_simplify.cc

  /*!
-   * \brief Detect if psum = q * coeff + r such that (q >= 0 && r >= 0)
+   * f\brief Separate psum into divisible and non-divisible parts.


I guess f at the beginning is a typo

sgrechanik-h · 2019-07-05T07:44:59Z

src/arithmetic/canonical_simplify.cc

+   * \param div_mode The new div_mode.
+   * \return The transformed SplitExpr.
+   */
+  SplitExpr ConvertDivMode(SplitExpr expr, DivMode div_mode) {


This function doesn't perform full conversion of one division into another, so it only works in simple cases. It is not clear from the description whether it is intentional or the function is just unfinished.

sgrechanik-h · 2019-07-05T10:11:18Z

src/arithmetic/const_fold.h

+        CHECK_NE(pb->value, 0) << "Divide by zero";
+      }
+      if (fa && fb && fb->value != 0) {
+        return FloatImm::make(rtype, std::floor(fa->value / fb->value));


I'm not sure that FloorDiv for floats should be allowed.

This is debatable. There used to be some need in terms of floordiv for floats(in tensorflow etc), given that this is a simple op, perhaps we could allow this.

Hzfengsy · 2019-07-04T23:25:11Z

src/arithmetic/int_op_overflow.h

+      (x % y == 0);
+  return round_down ? (x % y) : (x % y + y);
+}
+


I am not sure why these two functions are written in int_op_overflow.h

Hzfengsy · 2019-07-05T02:05:49Z

src/arithmetic/int_set.cc

+    if (analyzer->CanProveGreaterEqual(b->min_value, 0)) {
+      Expr min_value = a->HasLowerBound() ? floordiv(a->min_value, b->min_value) : neg_inf();
+      Expr max_value = a->HasUpperBound() ? floordiv(a->max_value, b->min_value) : pos_inf();
+      return IntervalSet(min_value, max_value);


What will happen if a with a negative min_value and a positive max_value? Does it will cause problems?

Because b is positive, the division of floordiv(a, b) is monotonic with respect to a, so it should be fine.

tqchen · 2019-07-06T18:48:09Z

Thanks, @sgrechanik-h @Hzfengsy , I have addressed your comments.

Hzfengsy · 2019-07-06T21:43:59Z

src/arithmetic/int_operator.h

- * \file int_op_overflow.h
- * \brief Utility functions to detect if an integer op will overflow.
+ * \file int_operator.h
+ * \brief Additional useful operators for intteger.


typo 'intteger'

yzhliu · 2019-07-08T16:23:28Z

src/arithmetic/int_operator.h

+ * \param y The right operand.
+ * \return the result.
+ */
+inline int64_t floordiv(int64_t x, int64_t y) {


shall we check y == 0?

tqchen · 2019-07-08T16:52:54Z

@sgrechanik-h @Hzfengsy please take another look and https://docs.tvm.ai/contribute/code_review.html#approve-and-request-changes-explicitly

sgrechanik-h · 2019-07-09T06:00:05Z

src/arithmetic/int_set.cc

+    }
+    if (is_one(b->min_value)) return a;
+    // no relaxation is needed in here due to set is inclusive
+    if (analyzer->CanProveGreaterEqual(b->min_value, 0)) {


I guess we have to compare with 1 here to avoid division by zero, like in the next if-branch. (Or compare with 0 and remove the check from the TryConstFold function since b->min_value == 0 may result from relaxation and shouldn't lead to failure).

good point, I added a condition to catch this problem

Oh, sorry, I forgot that we are in the b->IsSinglePoint() branch, so I guess my comment is meaningless since b->min_value == 0 definitely leads to division by zero in this case, and failing in TryConstFold shouldn't be a problem.
However, I think it is possible to replace analyzer->CanProveGreaterEqual(-b->min_value, 1) with analyzer->CanProveGreaterEqual(-b->min_value, 0) in the next branch, for symmetry.

OK, I will make the change

sgrechanik-h · 2019-07-10T13:26:53Z

src/arithmetic/rewrite_simplify.cc

-        (x / c1) * c1 + x % c1, x,
-        CanProveGreaterEqual(x.Eval(), 0) && c1.Eval()->value > 0);
+    // DivMod rules
+    // truc div


multiple truc/trunc typos in the file

tqchen · 2019-07-13T05:06:51Z

Thanks @Hzfengsy @sgrechanik-h @yzhliu this pr is now merged

* [ARITH][IR] Introduce FloorDiv/Mod * Address review comments * address review comments, fix div sub rule

tqchen added status: RFC status: need review labels Jul 3, 2019

sgrechanik-h reviewed Jul 5, 2019

View reviewed changes

Hzfengsy reviewed Jul 5, 2019

View reviewed changes

Hzfengsy reviewed Jul 6, 2019

View reviewed changes

tqchen force-pushed the arithx branch from 2383cfa to 7693592 Compare July 7, 2019 03:09

yzhliu approved these changes Jul 8, 2019

View reviewed changes

Hzfengsy approved these changes Jul 8, 2019

View reviewed changes

tqchen force-pushed the arithx branch from 7693592 to 9cf0140 Compare July 8, 2019 20:57

sgrechanik-h reviewed Jul 9, 2019

View reviewed changes

sgrechanik-h reviewed Jul 10, 2019

View reviewed changes

tqchen force-pushed the arithx branch 2 times, most recently from 2e67abf to 974286a Compare July 10, 2019 18:14

sgrechanik-h approved these changes Jul 11, 2019

View reviewed changes

tqchen added 3 commits July 12, 2019 15:41

[ARITH][IR] Introduce FloorDiv/Mod

c44b4a0

Address review comments

5aac752

address review comments, fix div sub rule

e811694

tqchen force-pushed the arithx branch from 974286a to e811694 Compare July 12, 2019 22:55

tqchen removed status: RFC status: need review labels Jul 13, 2019

tqchen merged commit 75892d2 into apache:master Jul 13, 2019

wweic pushed a commit to wweic/tvm that referenced this pull request Aug 9, 2019

[ARITH][IR] Introduce FloorDiv/Mod (apache#3479)

4251551

* [ARITH][IR] Introduce FloorDiv/Mod * Address review comments * address review comments, fix div sub rule

wweic pushed a commit to neo-ai/tvm that referenced this pull request Sep 6, 2019

[ARITH][IR] Introduce FloorDiv/Mod (apache#3479)

91baf68

* [ARITH][IR] Introduce FloorDiv/Mod * Address review comments * address review comments, fix div sub rule

tqchen mentioned this pull request Sep 20, 2019

[ARITH] Explicitly state truncdiv/mod in pattern matching. #3986

Merged

tqchen mentioned this pull request Nov 8, 2019

[RELEASE][DRAFT] TVM v0.6 Release candidate #4259

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ARITH][IR] Introduce FloorDiv/Mod #3479

[ARITH][IR] Introduce FloorDiv/Mod #3479

tqchen commented Jul 3, 2019

tqchen commented Jul 4, 2019 •

edited

Loading

sgrechanik-h Jul 5, 2019

sgrechanik-h Jul 5, 2019

sgrechanik-h Jul 5, 2019

sgrechanik-h Jul 5, 2019

sgrechanik-h Jul 5, 2019

tqchen Jul 6, 2019

Hzfengsy Jul 4, 2019

Hzfengsy Jul 5, 2019

tqchen Jul 6, 2019

tqchen commented Jul 6, 2019

Hzfengsy Jul 6, 2019

yzhliu Jul 8, 2019

tqchen commented Jul 8, 2019

sgrechanik-h Jul 9, 2019

tqchen Jul 9, 2019

sgrechanik-h Jul 10, 2019

tqchen Jul 10, 2019

sgrechanik-h Jul 10, 2019

tqchen commented Jul 13, 2019

[ARITH][IR] Introduce FloorDiv/Mod #3479

[ARITH][IR] Introduce FloorDiv/Mod #3479

Conversation

tqchen commented Jul 3, 2019

tqchen commented Jul 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Jul 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Jul 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Jul 13, 2019

tqchen commented Jul 4, 2019 •

edited

Loading