[RELAY][TypeSystem] Add support for populating type args #1962

jroesch · 2018-10-23T07:47:48Z

The RTS system PR #1954 requires that we correctly populate the type arguments field. This PR adds this behavior separately and is a prerequisite for #1954.

I also added support to the text printer which was tripping an assertion otherwise.

I added one test to show this works correctly for primitive operations.

jroesch · 2018-10-23T07:48:11Z

include/tvm/relay/expr.h

@@ -267,7 +267,7 @@ class CallNode : public ExprNode {
   *
   * \endcode
   */
-  tvm::Array<Type> type_args;
+  mutable tvm::Array<Type> type_args;


It may be better to use const cast selectively in the implementation rather then marking this field as mutable.

I agree using const cast selectively is a better approach

jroesch · 2018-10-23T07:48:46Z

cc @MarisaKirisame @tqchen

Add initial version of evaluator and tests WIP Work towards simple examples in the evaluator Requires implementation of lowering ops and monomorph Evaluator now works on simple cases Restore Function case in Evaluator WIP Fix rebase issues working towards working version RTS is now working again RTS can add numbers now Fix some rebase issues Fix up tests post rebase WIP Issue type checking MLP Remove dead file Clean up evaluator Remove accidental change Reset changes from apache#1962

tqchen · 2018-10-23T16:36:32Z

include/tvm/relay/expr.h

@@ -267,7 +267,7 @@ class CallNode : public ExprNode {
   *
   * \endcode
   */
-  tvm::Array<Type> type_args;
+  mutable tvm::Array<Type> type_args;


I agree using const cast selectively is a better approach

src/relay/ir/text_printer.cc

src/relay/pass/type_infer.cc

tqchen · 2018-10-23T16:41:34Z

src/relay/pass/type_infer.cc

+    auto call = GetRef<Call>(op);
+    auto it = tmap_.find(call);
+    if (it != tmap_.end()) {
+      Call new_op = Downcast<Call>(AttachCheckedType(op));


Maybe we can change AttachCheckedType -> AttachTypeInfo, and directly attach type_args when it is defined

tqchen · 2018-10-23T16:42:14Z

src/relay/pass/type_infer.cc

+
+  Type checked_type;
+  // Only allocated when the expression is a call.
+  Array<Type> type_args;


Array type_args(NodePtr(nullptr)) // specially initialize to undefined

src/relay/pass/type_infer.cc

tqchen · 2018-10-23T17:10:45Z

please fix the warnings here http://ci.tvm.ai:8080/blue/organizations/jenkins/tvm/detail/PR-1962/2/pipeline

MarisaKirisame · 2018-10-23T19:43:54Z

src/relay/ir/text_printer.cc

@@ -277,16 +277,30 @@ class TextPrinter :
  TextValue VisitExpr_(const CallNode* op) final {
    // TODO(tqchen, M.K.): support generic call


remove the todo

MarisaKirisame · 2018-10-23T19:48:51Z

src/relay/pass/type_infer.cc

+  void AddTypeArgs(const Expr& expr, Array<Type> type_args) {
+    auto type_info = type_map_.find(expr);
+    if (type_info == type_map_.end()) {
+      type_map_.insert({expr, ResolvedTypeInfo(type_args) });


remove extra space?

jroesch · 2018-10-23T23:20:36Z

@tqchen Should be good to go modulo CI.

include/tvm/relay/op.h

tqchen · 2018-10-23T23:28:51Z

src/relay/pass/type_infer.cc

+struct ResolvedTypeInfo {
+  explicit ResolvedTypeInfo(Type checked_type, Array<Type> type_args)
+      : checked_type(checked_type), type_args(type_args) {}
+  ResolvedTypeInfo(const ResolvedTypeInfo& rti)


likely you don't need this, as long as there are default constructor with no argument

tqchen · 2018-10-23T23:29:08Z

src/relay/pass/type_infer.cc

+      : checked_type(checked_type), type_args(type_args) {}
+  ResolvedTypeInfo(const ResolvedTypeInfo& rti)
+      : checked_type(rti.checked_type), type_args(rti.type_args) {}
+  ResolvedTypeInfo() : checked_type() {}


no need to initalized checked_type, it will pick the default constructor

I added these constructors due to a template instantiation error earlier.

Address feedback

Add initial version of evaluator and tests WIP Work towards simple examples in the evaluator Requires implementation of lowering ops and monomorph Evaluator now works on simple cases Restore Function case in Evaluator WIP Fix rebase issues working towards working version RTS is now working again RTS can add numbers now Fix some rebase issues Fix up tests post rebase WIP Issue type checking MLP Remove dead file Clean up evaluator Remove accidental change Reset changes from apache#1962 Rename Evaluator A little clean up WIP Clean up tests WIP WIP Repair from rebase and refactor to not use MM Remove testing code which is now in apache#1969 WIP

jroesch added 2 commits October 23, 2018 00:35

Add support for populating type args

e6abad2

Fix text printer and get test green

1660b31

jroesch commented Oct 23, 2018

View reviewed changes

Fix lint

d55cc31

jroesch changed the title ~~Add support for populating type args~~ [RELAY][TypeSystem] Add support for populating type args Oct 23, 2018

jroesch added a commit to jroesch/tvm that referenced this pull request Oct 23, 2018

Reset changes from apache#1962

1ed635a

tqchen requested changes Oct 23, 2018

View reviewed changes

src/relay/pass/type_infer.cc Show resolved Hide resolved

tqchen added the status: need update need update based on feedbacks label Oct 23, 2018

MarisaKirisame reviewed Oct 23, 2018

View reviewed changes

jroesch added 3 commits October 23, 2018 14:44

Addresss some comments

ca33fc2

Fix lint

21e2438

Fix test case

9dc874e

tqchen requested changes Oct 23, 2018

View reviewed changes

Fix printing for primitive ops

ba8c4cb

Address feedback

jroesch force-pushed the type_args_fix branch from 524bac2 to ba8c4cb Compare October 24, 2018 00:43

tqchen approved these changes Oct 24, 2018

View reviewed changes

tqchen merged commit 3bfa5fc into apache:master Oct 24, 2018

tqchen added status: accepted and removed status: need update need update based on feedbacks labels Oct 24, 2018

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

[RELAY][TypeSystem] Add support for populating type args (apache#1962)

23ed7f7

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

[RELAY][TypeSystem] Add support for populating type args (apache#1962)

4ff817d

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

[RELAY][TypeSystem] Add support for populating type args (apache#1962)

0cc8126

jroesch deleted the type_args_fix branch February 4, 2021 04:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RELAY][TypeSystem] Add support for populating type args #1962

[RELAY][TypeSystem] Add support for populating type args #1962

jroesch commented Oct 23, 2018

jroesch Oct 23, 2018

tqchen Oct 23, 2018

jroesch commented Oct 23, 2018

tqchen Oct 23, 2018

tqchen Oct 23, 2018

tqchen Oct 23, 2018

tqchen commented Oct 23, 2018

MarisaKirisame Oct 23, 2018

MarisaKirisame Oct 23, 2018

jroesch commented Oct 23, 2018

tqchen Oct 23, 2018

tqchen Oct 23, 2018

jroesch Oct 24, 2018

		@@ -277,16 +277,30 @@ class TextPrinter :
		TextValue VisitExpr_(const CallNode* op) final {
		// TODO(tqchen, M.K.): support generic call

[RELAY][TypeSystem] Add support for populating type args #1962

[RELAY][TypeSystem] Add support for populating type args #1962

Conversation

jroesch commented Oct 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jroesch commented Oct 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Oct 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jroesch commented Oct 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment