[TIR][REFACTOR][API-CHANGE] Change Call.name to Call.op(RelayExpr) #5863

tqchen · 2020-06-21T02:20:25Z

This PR brings a major refactor to the tir::Call structure.
The current Call structure uses a string field(name) to identify the
function/intrinsic being called. This approach is limited as we start
to expand TIR to be more structured. In particular, we are interested in
the following aspects:

Type a function and perform better compile time type checking so that we
can find errors early.
Register additional properties about an operator, such as:
- Whether an intrinsic can be vectorized
- What is the adjoint function of the intrinsic(for tensor expression AD)
- Whether the operator has side effect.
Perform specific codegen about an intrinsic if necessary.
Call into another function in the same module.

The refactor changes the Call.name field to Call.op.
The Call.op field has a RelayExpr type, and we can pass:

A tvm::Op which represents the corresponding intrinsic.
A tvm::GlobalVar for calling into another function in the IRModule.

All the current intrinsics are migrated by registering an tvm::Op.
Because the unified IR shares a single Op registry. We use the "tir"
namespace for tir related intrinsics, for example bitwise and is now registered
under tir.bitwise_and.

To simplify upgrade, we introduce tir.call_extern intrinsic
that allows us to call into arbitary external function without type checking.
However, we should move towards more type checked variants in the system.

Under the new op design. We should no longer try to pattern match all the
specific intrincis. Instead, we should rely on attr of each Op to do transformation.
For example, the vectorization pass depends on the TVectorizable property of the op,
which can be registered independently.

In this way, we can still grow the number of intrinsics when necessary
without having to change all the passes.

The same rule applies for tensor expression AD. Currently we are performing
AD by pattern match on operators like exp, sin, cos. We should instead
change to the ajoint registeration mechanism like those in relay.

Followup refactors need to be performed, including:

Fold the Call.call_type into operator's attribute.
Enrich the operator registry information
Refactor passes(e.g. AD, intrin lowering) to use the attribute based transformation

Upgrade Note

If you are using an raw intrinsic API, likely we need to add the tir. prefix to it.

call_pure_intrin(x.dtype, 'tir.exp', [x])

junrushao · 2020-06-21T07:36:47Z

I like the idea how it organizes intrins. Is this PR ready for review?

…Op/RelayExpr) This PR brings a major refactor to the tir::Call structure. The current Call structure uses a string field(name) to identify the function/intrinsic being called. This approach is limited as we start to expand TIR to be more structured. In particular, we are interested in the following aspects: - Type a function and perform better compile time type checking so that we can find errors early. - Register additional properties about an operator, such as: - Whether an intrinsic can be vectorized - What is the adjoint function of the intrinsic(for tensor expression AD) - Whether the operator has side effect. - Perform specific codegen about an intrinsic if necessary. - Call into another function in the same module. The refactor changes the Call.name field to Call.op. The Call.op field has a RelayExpr type, and we can pass: - A tvm::Op which represents the corresponding intrinsic. - A tvm::GlobalVar for calling into another function in the IRModule. All the current intrinsics are migrated by registering an tvm::Op. Because the unified IR shares a single Op registry. We use the "tir" namespace for tir related intrinsics, for example bitwise and is now registered under `tir.bitwise_and`. To simplify upgrade, we introduce a `tir.call_extern` intrinsic that allows us to call into arbitary external function without type checking. However, we should move towards more type checked variants in the system. Under the new op design. We should no longer try to pattern match all the specific intrincis. Instead, we should rely on attr of each Op to do transformation. For example, the vectorization pass depends on the TVectorizable property of the op, which can be registered independently. In this way, we can still grow the number of intrinsics when necessary without having to change all the passes. The same rule applies for tensor expression AD. Currently we are performing AD by pattern match on operators like exp, sin, cos. We should instead change to the ajoint registeration mechanism like those in relay. Followup refactors need to be performed, including: - Fold the Call.call_type into operator's attribute. - Enrich the operator registry information - Refactor passes(e.g. AD, intrin lowering) to use the attribute based transformation

tqchen · 2020-06-21T16:44:42Z

cc @junrushao1994 @yzhliu @merrymercy @ZihengJiang @wpan11nv @yongfeng-nv @masahi @Hzfengsy @spectrometerHBH @xqdan @FrozenGene @antinucleon @vinx13 @jwfromm

yzhliu

The overall design and the ad part change look good to me.

junrushao · 2020-06-22T20:18:03Z

include/tvm/tir/expr.h

+   *  - It can be tvm::Op which corresponds to the primitive operators(intrinsics).
+   *  - It can also be another function in the IRModule (GlobalVar).
+   */
+  RelayExpr op;


Do you think we should someday move GlobalVar out of RelayExpr?

Any suggested alternatives and rationale?

include/tvm/tir/op_attr_types.h

python/tvm/contrib/nvcc.py

junrushao · 2020-06-22T21:14:11Z

src/target/source/intrin_rule_cuda.cc

    }
-    return "";
+    return Op(nullptr);


shall we just LOG(FATAL) here and throw?

junrushao · 2020-06-22T21:21:00Z

I have a question on TVectorizable. I think it is possible that an intrinsic is vectorizable as one dtype, but not vectorizable as another dtype. Is that correct?

junrushao · 2020-06-22T21:23:16Z

src/tir/op/builtin.cc

+
+TIR_DEFINE_BUILTIN_FUNC(tvm_stack_make_array).set_num_inputs(6);
+
+// When num_inputs are not set, the function is assumed to be variable length.


Do we have a checker somewhere in the codebase on the number of inputs?

Not at the moment, so far we are only providing these information without checking them to ease migration. A checker is certainly an important followup step

tqchen · 2020-06-22T21:24:13Z

@junrushao1994 you are right. There are two ways to deal with such situation:

Force most TIR intrincis to always be vectorizable, the legalize later(by scalarize)
Add more info to the registry to add this information.

The current PR simply migrates the previous code in a minimum way, and we should perform follow ups along this two directions

junrushao · 2020-06-22T21:30:02Z

tests/cpp/ir_functor_test.cc

    auto res = v(std::move(body));
-    CHECK(res.as<EvaluateNode>()->value.as<CallNode>()->args[0].same_as(x));
+    CHECK(res.as<EvaluateNode>()->value.as<CallNode>()->args[1].same_as(x));


Is this intended?

Okay it is...just ignore me

junrushao · 2020-06-22T21:36:57Z

@tqchen I am in favor of the second solution. For example, we can change TVectorizable to FVectorizable, which accepts an argument dtype, and returns a boolean indicating whether this intrinsic is vectorizable under this dtype.

junrushao

LGTM

…Expr) (apache#5863)" This reverts commit 82d157f.

tqchen · 2020-06-26T23:00:13Z

Followup PR #5937

…pache#5863) * [TIR][REFACTOR][API-CHANGE] Change Call.name(string) to Call.op(tvm::Op/RelayExpr) This PR brings a major refactor to the tir::Call structure. The current Call structure uses a string field(name) to identify the function/intrinsic being called. This approach is limited as we start to expand TIR to be more structured. In particular, we are interested in the following aspects: - Type a function and perform better compile time type checking so that we can find errors early. - Register additional properties about an operator, such as: - Whether an intrinsic can be vectorized - What is the adjoint function of the intrinsic(for tensor expression AD) - Whether the operator has side effect. - Perform specific codegen about an intrinsic if necessary. - Call into another function in the same module. The refactor changes the Call.name field to Call.op. The Call.op field has a RelayExpr type, and we can pass: - A tvm::Op which represents the corresponding intrinsic. - A tvm::GlobalVar for calling into another function in the IRModule. All the current intrinsics are migrated by registering an tvm::Op. Because the unified IR shares a single Op registry. We use the "tir" namespace for tir related intrinsics, for example bitwise and is now registered under `tir.bitwise_and`. To simplify upgrade, we introduce a `tir.call_extern` intrinsic that allows us to call into arbitary external function without type checking. However, we should move towards more type checked variants in the system. Under the new op design. We should no longer try to pattern match all the specific intrincis. Instead, we should rely on attr of each Op to do transformation. For example, the vectorization pass depends on the TVectorizable property of the op, which can be registered independently. In this way, we can still grow the number of intrinsics when necessary without having to change all the passes. The same rule applies for tensor expression AD. Currently we are performing AD by pattern match on operators like exp, sin, cos. We should instead change to the ajoint registeration mechanism like those in relay. Followup refactors need to be performed, including: - Fold the Call.call_type into operator's attribute. - Enrich the operator registry information - Refactor passes(e.g. AD, intrin lowering) to use the attribute based transformation * Fix nms * Fix remaining testcase * Address review comment

tqchen force-pushed the call-op-refactor branch 3 times, most recently from a87289c to 1190dec Compare June 21, 2020 04:50

tqchen force-pushed the call-op-refactor branch from dbc0777 to 3c32760 Compare June 21, 2020 16:23

tqchen mentioned this pull request Jun 21, 2020

Track Refactor Items and Update Guide #4647

Closed

tqchen and others added 2 commits June 21, 2020 10:50

Fix nms

3505e72

Fix remaining testcase

18067de

ZihengJiang approved these changes Jun 22, 2020

View reviewed changes

yzhliu approved these changes Jun 22, 2020

View reviewed changes

junrushao reviewed Jun 22, 2020

View reviewed changes

Address review comment

112f3b7

junrushao reviewed Jun 22, 2020

View reviewed changes

junrushao approved these changes Jun 22, 2020

View reviewed changes

tqchen merged commit 82d157f into apache:master Jun 23, 2020

windclarion pushed a commit to windclarion/incubator-tvm that referenced this pull request Jun 23, 2020

Revert "[TIR][REFACTOR][API-CHANGE] Change Call.name to Call.op(Relay…

d770741

…Expr) (apache#5863)" This reverts commit 82d157f.

tqchen mentioned this pull request Jun 26, 2020

[TIR][OP][API-CHANGE] Remove CallNode.call_type in favor of attribute. #5937

Merged

tqchen mentioned this pull request Jun 29, 2020

[TASK] Sync with Upstream tlc-pack/tvm-tensorir#63

Closed

ZihengJiang mentioned this pull request Sep 25, 2020

TVM v0.7 Release Note Candidate #6486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TIR][REFACTOR][API-CHANGE] Change Call.name to Call.op(RelayExpr) #5863

[TIR][REFACTOR][API-CHANGE] Change Call.name to Call.op(RelayExpr) #5863

tqchen commented Jun 21, 2020 •

edited

Loading

junrushao commented Jun 21, 2020

tqchen commented Jun 21, 2020

yzhliu left a comment

junrushao Jun 22, 2020

tqchen Jun 22, 2020

junrushao Jun 22, 2020

junrushao commented Jun 22, 2020

junrushao Jun 22, 2020

tqchen Jun 22, 2020

tqchen commented Jun 22, 2020

junrushao Jun 22, 2020

junrushao Jun 22, 2020

junrushao commented Jun 22, 2020

junrushao left a comment

tqchen commented Jun 26, 2020 •

edited

Loading


		TIR_DEFINE_BUILTIN_FUNC(tvm_stack_make_array).set_num_inputs(6);

		// When num_inputs are not set, the function is assumed to be variable length.

[TIR][REFACTOR][API-CHANGE] Change Call.name to Call.op(RelayExpr) #5863

[TIR][REFACTOR][API-CHANGE] Change Call.name to Call.op(RelayExpr) #5863

Conversation

tqchen commented Jun 21, 2020 • edited Loading

Upgrade Note

junrushao commented Jun 21, 2020

tqchen commented Jun 21, 2020

yzhliu left a comment

Choose a reason for hiding this comment

junrushao Jun 22, 2020

Choose a reason for hiding this comment

tqchen Jun 22, 2020

Choose a reason for hiding this comment

junrushao Jun 22, 2020

Choose a reason for hiding this comment

junrushao commented Jun 22, 2020

junrushao Jun 22, 2020

Choose a reason for hiding this comment

tqchen Jun 22, 2020

Choose a reason for hiding this comment

tqchen commented Jun 22, 2020

junrushao Jun 22, 2020

Choose a reason for hiding this comment

junrushao Jun 22, 2020

Choose a reason for hiding this comment

junrushao commented Jun 22, 2020

junrushao left a comment

Choose a reason for hiding this comment

tqchen commented Jun 26, 2020 • edited Loading

tqchen commented Jun 21, 2020 •

edited

Loading

tqchen commented Jun 26, 2020 •

edited

Loading