[IR] eager constant folding in operator overloading #1789

tqchen · 2018-10-01T03:28:00Z

Make sure Expr now eargerly folds constant when it is integer expression, removes the need to call simplify in all concrete shape inference case so that we can reuse symbolic integer for concrete inference.

Example

x = tvm.const(1)
y = x + 1
assert isinstance(y, tvm.expr.IntImm) and y.value == 2

tqchen · 2018-10-01T03:35:35Z

@yzhliu @MarisaKirisame @junrushao1994 can you help do a quick round of review?

zhiics · 2018-10-01T17:46:12Z

src/lang/ir_operator.cc

+  const IntImm* pa = a.as<IntImm>();                                    \
+  const IntImm* pb = b.as<IntImm>();                                    \
+  const Type& ta = a.type();                                            \
+  const Type& tb = a.type();                                            \


tb = b.type()?

zhiics · 2018-10-01T17:56:20Z

include/tvm/ir_operator.h

+ * \tparam ValueType The constant value type
+ */
+template<typename ValueType,
+         typename = typename std::enable_if<std::is_pod<ValueType>::value>::type>


Is std::is_arithmetic enough here? Seems std::is_pod will be deprecated.

zhiics · 2018-10-01T18:10:22Z

include/tvm/ir_operator.h

+TVM_DEFINE_INT_OP_CONST_VAL_OVERLOAD(operator<<); // NOLINT(*)
+TVM_DEFINE_INT_OP_CONST_VAL_OVERLOAD(operator&);
+TVM_DEFINE_INT_OP_CONST_VAL_OVERLOAD(operator|);
+TVM_DEFINE_INT_OP_CONST_VAL_OVERLOAD(operator^);


Just curious, do we need ~ op?

this macro defines operator for binary ops, ~ is already defined as single operand op

zhiics · 2018-10-01T18:12:52Z

src/arithmetic/compute_expr.h

-
-template<>
-inline bool GetConst<int64_t>(Expr e, int64_t *out) {
+inline bool GetConst(Expr e, int64_t *out) {


Nit: int64_t* out

zhiics · 2018-10-01T18:14:33Z

src/lang/ir_operator.cc

+/*!
+ * \brief Check whether type is used to represent index.
+ *
+ * Index type are frequently used in shape computation


zhiics · 2018-10-01T18:15:14Z

src/lang/ir_operator.cc

+}
+
+// The public function with a quick checking path.
+void BinaryOpMatchTypes(Expr &lhs, Expr &rhs) {  // NOLINT(*)


tqchen · 2018-10-01T18:54:37Z

Thanks @zhiics for very helpful comments, i have fixed them, can you please take another round of look?

MarisaKirisame · 2018-10-01T18:44:47Z

include/tvm/ir_operator.h

+
+/*!
+ * \brief Check whether x is a constant power of two
+ * \note This only return true for integer types.


what is shift? I assume it is 'if it is const pow of 2, write to the pointer'
if so please add document

MarisaKirisame · 2018-10-01T18:52:34Z

src/lang/ir_operator.cc

+  TVM_CONST_PROPAGATION({
+      Type rtype = ta.bits() >= tb.bits() ? ta : tb;
+      if (pa && pb) return IntImm::make(rtype, pa->value - pb->value);
+      if (pb && pb->value == 0) return SimpleCast(rtype, a);


if pa is 0, use unary neg?

There is no unary neg in the expression

MarisaKirisame · 2018-10-01T18:53:22Z

src/lang/ir_operator.cc

+      if (pb) {
+        if (pb->value == 1) return SimpleCast(rtype, a);
+      }
+    });


check for x/0?

MarisaKirisame · 2018-10-01T18:53:43Z

src/lang/ir_operator.cc

+Expr operator/(Expr a, Expr b) {
+  TVM_CONST_PROPAGATION({
+      Type rtype = ta.bits() >= tb.bits() ? ta : tb;
+      // due to division and mode can have different modes


division and mod

MarisaKirisame · 2018-10-01T18:54:01Z

src/lang/ir_operator.cc

+Expr operator%(Expr a, Expr b) {
+  TVM_CONST_PROPAGATION({
+      Type rtype = ta.bits() >= tb.bits() ? ta : tb;
+      // due to division and mode can have different modes


division and mod

MarisaKirisame · 2018-10-01T18:54:09Z

src/lang/ir_operator.cc

+        if (pa->value == 0) return SimpleCast(rtype, a);
+      }
+      if (pb) {
+        if (pb->value == 1) return make_zero(rtype);


check for 0?

MarisaKirisame · 2018-10-01T18:56:12Z

src/lang/ir_operator.cc

+      return false_value;
+    }
+  }
+  CHECK(cond.type().is_bool());


move check before if (so early return still get checked)?

MarisaKirisame · 2018-10-01T18:58:51Z

src/lang/ir_operator.cc

+  } else if (x.type().is_uint()) {
+    return x;
+  } else {
+    LOG(WARNING) << "Warning: Data type " << x.type()


why warning? what other case is there?

MarisaKirisame · 2018-10-01T18:59:32Z

src/pass/split_pipeline.cc

      }
-      alloc_size = ir::Simplify(alloc_size);
+      alloc_size = alloc_size;


redundant line

tqchen · 2018-10-01T19:18:02Z

Thanks @MarisaKirisame for the comments, I have fixed accordingly

zhiics · 2018-10-01T20:30:42Z

@tqchen LGTM. I have the same question again, will/should we have reach-def and/or liveness analysis in Relay? We might want them to help some data flow related opts.

tqchen · 2018-10-01T20:39:21Z

@zhiics it might make sense to do so, but I would like to see such pass driven by a real end to end usecase, e.g. memory allocation pass

tqchen · 2018-10-01T20:39:49Z

@zhiics @MarisaKirisame can you please https://docs.tvm.ai/contribute/code_review.html#approve-and-request-changes-explicitly

tqchen · 2018-10-01T20:46:03Z

Thanks @MarisaKirisame @junrushao1994 @zhiics this is now merged

sgrechanik-h · 2018-10-04T13:44:41Z

include/tvm/ir_operator.h

+ */
+inline bool is_zero(const Expr& x) {
+  return is_const_int(x, 0);
+}


This is confusing. HalideIR contains a function with the same name that works for floats too. Moreover, it used to be reexported into the tvm namespace, so this change might have broken something in a very subtle way, like performance regression. (Hopefully, it didn't, because in tvm is_zero seems to be used only for ints indeed, but anyway). I would consider renaming it or throwing an exception if the argument is not integer.

[IR] enable eager constant folding in default operators

80d0ddf

fix cpptest

558b3ed

tqchen changed the title ~~[IR] enable eager constant folding in default operators~~ [IR] enable eager constant folding in operator overloading Oct 1, 2018

tqchen changed the title ~~[IR] enable eager constant folding in operator overloading~~ [IR] eager constant folding in operator overloading Oct 1, 2018

tqchen added 2 commits September 30, 2018 21:13

recover most type conversion rule

914876e

fix testcase

144453c

zhiics reviewed Oct 1, 2018

View reviewed changes

fix review comment

09dcfe1

MarisaKirisame reviewed Oct 1, 2018

View reviewed changes

fix comment by Marisa

c3b3013

MarisaKirisame approved these changes Oct 1, 2018

View reviewed changes

junrushao approved these changes Oct 1, 2018

View reviewed changes

tqchen merged commit 32af4d2 into apache:master Oct 1, 2018

sgrechanik-h reviewed Oct 4, 2018

View reviewed changes

FrozenGene pushed a commit to FrozenGene/tvm that referenced this pull request Dec 27, 2018

[IR] eager constant folding in operator overloading (apache#1789)

54dcfe3

Lunderberg mentioned this pull request Apr 26, 2022

[TIR] Reduced duplication in op.h #11129

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IR] eager constant folding in operator overloading #1789

[IR] eager constant folding in operator overloading #1789

tqchen commented Oct 1, 2018 •

edited

tqchen commented Oct 1, 2018 •

edited

zhiics Oct 1, 2018

zhiics Oct 1, 2018

zhiics Oct 1, 2018

tqchen Oct 1, 2018

zhiics Oct 1, 2018

zhiics Oct 1, 2018

zhiics Oct 1, 2018

tqchen commented Oct 1, 2018

MarisaKirisame Oct 1, 2018

MarisaKirisame Oct 1, 2018

tqchen Oct 1, 2018

MarisaKirisame Oct 1, 2018

MarisaKirisame Oct 1, 2018

MarisaKirisame Oct 1, 2018

MarisaKirisame Oct 1, 2018

MarisaKirisame Oct 1, 2018

MarisaKirisame Oct 1, 2018

MarisaKirisame Oct 1, 2018

tqchen commented Oct 1, 2018

zhiics commented Oct 1, 2018

tqchen commented Oct 1, 2018

tqchen commented Oct 1, 2018

tqchen commented Oct 1, 2018

sgrechanik-h Oct 4, 2018

[IR] eager constant folding in operator overloading #1789

[IR] eager constant folding in operator overloading #1789

Conversation

tqchen commented Oct 1, 2018 • edited

tqchen commented Oct 1, 2018 • edited

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 Oct 1, 2018

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

Choose a reason for hiding this comment

tqchen commented Oct 1, 2018

zhiics commented Oct 1, 2018

tqchen commented Oct 1, 2018

tqchen commented Oct 1, 2018

tqchen commented Oct 1, 2018

Choose a reason for hiding this comment

tqchen commented Oct 1, 2018 •

edited

tqchen commented Oct 1, 2018 •

edited