Defined a common base class for TensorComputeOp and ComputeOp #2587

derisavi · 2019-02-12T00:18:45Z

I renamed the current ComputeOp to ScalarComputeOp and created a base class and called it ComputeOp that both ScalarComputeOp and TensorComputeOp derive from. I also created a new virtual method (in ComputeOp) called num_schedulable_dims(). All names are the first sensible things that came to my mind. Please feel free to suggest improvements to naming, design, etc.

derisavi · 2019-02-12T00:26:39Z

This fixes #2583 . @ZihengJiang please take a look as you were the original developer of TensorComputeOp. Also @xqdan and @yzhliu might be interested. Thank you all.

xqdan · 2019-02-12T13:50:02Z

include/tvm/operation.h

+  void GatherBound(
+          const Operation& self,
+          const std::unordered_map<Tensor, TensorDom>& tensor_dom,
+          std::unordered_map<IterVar, Range>* out_dom_map) const final;


Since ComputeOpNode is an exposed structure, can we keep it as the original, but we have another BasicComputeOpNode; ComputeOpNode and TensorComputeOpNode inherit it. in this way, we can avoid so many modifications, also we can save modifications for those in-house projects based on tvm.

The way I see it is that TVM has defined a Node class, then derived OperationNode, then derived <X>OpNode, and so on. Therefore, before this PR, one can find out the class hierarchy only by looking at the names of the classes. I believe this was a good decision and I have seen it in other code bases. That's why I thought to keep it that way after this new class is introduced.

Regarding the amount of modifications needed both inside and outside TVM:

inside TVM: I agree that quite a few files have been affected. However, the changes are trivial, and have already been done fairly quickly using automatic refactoring tools so no additional effort is needed.

outside TVM: even if there are a lot of code out there using ComputeOpNode (which need to be renamed to ScalarComputeOpNode), I don't see why it's an inconvenient thing to do that. Maybe I missing something?
Moreover, I think keeping the consistency of class naming in TVM (which I hope strives for many years to come) is more important than that level of inconvenience in external projects that depend on it.

That's my two cents.

The way I see it is that TVM has defined a Node class, then derived OperationNode, then derived <X>OpNode, and so on. Therefore, before this PR, one can find out the class hierarchy only by looking at the names of the classes. I believe this was a good decision and I have seen it in other code bases. That's why I thought to keep it that way after this new class is introduced.

agree. what i'm suggesting has no conflicts with this point.

Regarding the amount of modifications needed both inside and outside TVM:

inside TVM: I agree that quite a few files have been affected. However, the changes are trivial, and have already been done fairly quickly using automatic refactoring tools so no additional effort is needed.

outside TVM: even if there are a lot of code out there using ComputeOpNode (which need to be renamed to ScalarComputeOpNode), I don't see why it's an inconvenient thing to do that. Maybe I missing something?

We do have some custmized pass(C++ and python) and costmized schedule template(python) , in which we referenced ComputeOpNode and the code is not in public, if this is merged, we need to replace them with ScalarComputeOpNode when sync with upstream. I believe other inhouse projects may have the similar issue with this.

Moreover, I think keeping the consistency of class naming in TVM (which I hope strives for many years to come) is more important than that level of inconvenience in external projects that depend on it.

I don't see much differences if we change ComputeOpNode to ScalarComputeOpNode, since everybody who uses ComputeOpNode knew it's a scalar compute, for those who don't know, a comment on ComputeOpNode class will be enough.

That's my two cents.

Thanks @xqdan. I followed the naming scheme you recommended. I used BaseComputeOpNode for the base class.

xqdan

thanks, @derisavi , looks good to me now

ZihengJiang · 2019-02-21T23:51:56Z

include/tvm/operation.h

+          const Stmt& body) const final;
+  virtual size_t num_schedulable_dims() const = 0;
+
+  static constexpr const char* _type_key = "ComputeOp";


BaseComputeOp here

ZihengJiang · 2019-02-21T23:52:36Z

include/tvm/operation.h

@@ -185,21 +185,42 @@ class PlaceholderOpNode : public OperationNode {
 /*!
 * \brief A Compute op that compute a tensor on certain domain.


stressing that it is the base class of ComputeOp and TensorComputeOp

ZihengJiang · 2019-02-21T23:58:34Z

Hi @derisavi , it would be better if you can provide a test case to demonstrate using tensorize with TensorComputeOp, which is mentioned in your RFC.

derisavi · 2019-02-22T18:47:17Z

Hi @derisavi , it would be better if you can provide a test case to demonstrate using tensorize with TensorComputeOp, which is mentioned in your RFC.

That RFC (#2583) is only trying to motivate having a common base class and as one of the reasons I've mentioned tensorization. I think tensorization of TensorComputeOp requires its own separate RFC issue. Should I then just create a new issue for that and a testcase there?

ZihengJiang · 2019-02-22T19:38:18Z

@derisavi If tensorization of TensorComputeOp need other modification for code, we can leave it to another PR

derisavi · 2019-02-22T20:10:25Z

@ZihengJiang We have tried to tensorize TensorComputeOp and it doesn't work out of the box. It does need more modifications. Should I still put the testcase?

ZihengJiang · 2019-02-22T20:28:32Z

@derisavi Then we can aim at supporting it in this PR, or make sure tensorize will throw error for TensorComputeOp

derisavi · 2019-02-22T22:17:21Z

or make sure tensorize will throw error for TensorComputeOp
@ZihengJiang That's a good point. I missed that. When I tried tensorizing TensorComputeOp, TVM ignored the tensorize schedule API (i.e., as if it didn't exist). It didn't give any error messages.

Now I'm thinking of how to write such a testcase. Does it make sense to test for correctness (i.e., if tensorize works) by asserting that the axis variable that we have tensorized should NOT exist in the program after ScheduleOps?

ZihengJiang · 2019-02-22T23:03:17Z

@derisavi sounds good, you can also check the testcase for tensorize: https://github.com/dmlc/tvm/blob/master/tests/python/unittest/test_lang_schedule.py#L172

…on TensorComputeOp ops.

derisavi · 2019-02-25T19:58:23Z

@ZihengJiang I added a testcase that asserts that tensorize of a loop of a TensorComputeOp has no effect, i.e., the loop that is supposed to be tensorized will not be replaced by anything.
The testcase is not complete in the sense that it doesn't generate meaningful code if we eventually support tensorization for TensorComputeOp. When we start supporting that type of tensorization, the testcase will fail, and we need to modify it.

ZihengJiang · 2019-03-01T18:46:31Z

Merged, thanks @derisavi

@ZihengJiang

…#2587) * Defined a common base class for TensorComputeOp and ComputeOp * Made changes requested by @ZihengJiang * added a testcase to assert that `tensorize` does not have any effect on TensorComputeOp ops.

@ZihengJiang

…#2587) * Defined a common base class for TensorComputeOp and ComputeOp * Made changes requested by @ZihengJiang * added a testcase to assert that `tensorize` does not have any effect on TensorComputeOp ops.

@ZihengJiang

…#2587) * Defined a common base class for TensorComputeOp and ComputeOp * Made changes requested by @ZihengJiang * added a testcase to assert that `tensorize` does not have any effect on TensorComputeOp ops.

@ZihengJiang

…#2587) * Defined a common base class for TensorComputeOp and ComputeOp * Made changes requested by @ZihengJiang * added a testcase to assert that `tensorize` does not have any effect on TensorComputeOp ops.

derisavi force-pushed the refactor_compute_op branch 4 times, most recently from 21a4457 to 621fc03 Compare February 12, 2019 03:05

tqchen added the status: need review label Feb 12, 2019

xqdan reviewed Feb 12, 2019

View reviewed changes

derisavi force-pushed the refactor_compute_op branch from 621fc03 to ca3d8d2 Compare February 14, 2019 19:13

xqdan approved these changes Feb 15, 2019

View reviewed changes

ZihengJiang self-assigned this Feb 17, 2019

ZihengJiang added status: need RFC need RFC discussion and removed status: need RFC need RFC discussion labels Feb 17, 2019

ZihengJiang reviewed Feb 21, 2019

View reviewed changes

derisavi added 2 commits February 22, 2019 13:38

Defined a common base class for TensorComputeOp and ComputeOp

b81a9fb

Made changes requested by @ZihengJiang

c618e5c

derisavi force-pushed the refactor_compute_op branch from ca3d8d2 to c618e5c Compare February 22, 2019 18:41

added a testcase to assert that tensorize does not have any effect …

d1de651

…on TensorComputeOp ops.

ZihengJiang approved these changes Mar 1, 2019

View reviewed changes

ZihengJiang merged commit d546bb7 into apache:master Mar 1, 2019

ZihengJiang added status: accepted and removed status: need review labels Mar 1, 2019

derisavi mentioned this pull request Mar 6, 2019

[RFC][TVM] Define a common base class for ComputeOpNode and TensorComputeOpNode #2583

Closed

derisavi deleted the refactor_compute_op branch April 18, 2019 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Defined a common base class for TensorComputeOp and ComputeOp #2587

Defined a common base class for TensorComputeOp and ComputeOp #2587

derisavi commented Feb 12, 2019

derisavi commented Feb 12, 2019

xqdan Feb 12, 2019

derisavi Feb 13, 2019

xqdan Feb 14, 2019 •

edited

Loading

derisavi Feb 14, 2019

xqdan left a comment

ZihengJiang Feb 21, 2019

derisavi Feb 22, 2019

ZihengJiang Feb 21, 2019

derisavi Feb 22, 2019

ZihengJiang commented Feb 21, 2019

derisavi commented Feb 22, 2019

ZihengJiang commented Feb 22, 2019

derisavi commented Feb 22, 2019

ZihengJiang commented Feb 22, 2019

derisavi commented Feb 22, 2019

ZihengJiang commented Feb 22, 2019

derisavi commented Feb 25, 2019

ZihengJiang commented Mar 1, 2019

		@@ -185,21 +185,42 @@ class PlaceholderOpNode : public OperationNode {
		/*!
		* \brief A Compute op that compute a tensor on certain domain.

Defined a common base class for TensorComputeOp and ComputeOp #2587

Defined a common base class for TensorComputeOp and ComputeOp #2587

Conversation

derisavi commented Feb 12, 2019

derisavi commented Feb 12, 2019

xqdan Feb 12, 2019

Choose a reason for hiding this comment

derisavi Feb 13, 2019

Choose a reason for hiding this comment

xqdan Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

derisavi Feb 14, 2019

Choose a reason for hiding this comment

xqdan left a comment

Choose a reason for hiding this comment

ZihengJiang Feb 21, 2019

Choose a reason for hiding this comment

derisavi Feb 22, 2019

Choose a reason for hiding this comment

ZihengJiang Feb 21, 2019

Choose a reason for hiding this comment

derisavi Feb 22, 2019

Choose a reason for hiding this comment

ZihengJiang commented Feb 21, 2019

derisavi commented Feb 22, 2019

ZihengJiang commented Feb 22, 2019

derisavi commented Feb 22, 2019

ZihengJiang commented Feb 22, 2019

derisavi commented Feb 22, 2019

ZihengJiang commented Feb 22, 2019

derisavi commented Feb 25, 2019

ZihengJiang commented Mar 1, 2019

xqdan Feb 14, 2019 •

edited

Loading