[BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass #6655

d-smirnov · 2020-10-09T19:19:57Z

This PR adds "include_non_call_ops" parameter to AnnotateTarget pass. When set (include_non_call_ops=True) the AnnotateTarget pass will annotate non call ops with default target or with the target of its arguments. This is current behavior of AnnotateTarget pass. When the flag is set to false (include_non_call_ops=False) the AnnotateTarget pass will not annotate non-call. This behavior is useful if you are not running MergeCompilerRegions pass after AnnotateTarget.

The PR related to an issue reported here: https://discuss.tvm.apache.org/t/arm-compute-library-segv-with-inception-v1-squeezenet/7985/8

@comaniac @manupa-arm @mbaret

comaniac · 2020-10-09T19:40:48Z

Thanks for the PR. I have a hard deadline this weekend and will review it after next Tuesday.

manupak

Thanks for this! some minor comments.

src/relay/transforms/annotate_target.cc

tests/python/relay/test_pass_annotate_target.py

python/tvm/relay/transform/transform.py

tests/python/relay/test_pass_annotate_target.py

comaniac · 2020-10-13T16:41:01Z

This solution looks too ad hoc to me. From the semantic of the changes, I think this PR can be generalized to "improve the annotate target pass to annotate non-call ops to default". Accordingly, the API could be like

AnnotateTarget(targets, skip_non_call_ops=False)(mod) # I am bad at naming but it is the point

cc @zhiics

python/tvm/relay/transform/transform.py

d-smirnov · 2020-11-15T17:25:40Z

@tqchen Hi, I am trying to compile Mobilnet (after expanded this patch to all non-call ops, not committed yet) and encountered a check on src/relay/backend/compile_engine.cc:170
ICHECK(op->is_scalar());

The ConstantNode it fails is of TensorType: TensorType([1, 1, 1024, 1024], float32), and contains runtime.NDArray(0x74f0370) of corresponding shape (1x1x1024x1024). Could you explain the purpose of the check and should the check be extended to also accomodate ConstantNodes of TensorType. I am also interested to know the techniques to trace when and why this ConstantNode appeared.

Thank you. cc: @comaniac

trevor-m · 2020-11-15T17:27:49Z

@tqchen Hi, I am trying to compile Mobilnet (after expanded this patch to all non-call ops, not committed yet) and encountered a check on src/relay/backend/compile_engine.cc:170
ICHECK(op->is_scalar());

The ConstantNode it fails is of TensorType: TensorType([1, 1, 1024, 1024], float32), and contains runtime.NDArray(0x74f0370) of corresponding shape (1x1x1024x1024). Could you explain the purpose of the check and should the check be extended to also accomodate ConstantNodes of TensorType. I am also interested to know the techniques to trace when and why this ConstantNode appeared.

Thank you. cc: @comaniac

Hey @d-smirnov have you tried with this change? #6912

d-smirnov · 2020-11-15T19:26:56Z

Hey @d-smirnov have you tried with this change? #6912

@trevor-m Rebased, but it did not make any difference. Still fail on the check src/relay/backend/compile_engine.cc:170

Added annotate_non_call_ops parameter to AnnotateTarget pass to prevent non-call to be promoted to previously annotated operations This is useful in case if you are not running MergeCompilerRegions pass after AnnotateTarget.

d-smirnov · 2020-11-20T20:31:18Z

Updated. Please take a look @comaniac, @mbaret, @manupa-arm

comaniac

I still think we should have separate postprocess logic to deal with the subgraph (a.k.a. region) without call nodes instead of coupling it to AnnotateTarget (similar to the pruning pass in TensorRT integration). The AnnotateTarget pass is already too complicate to maintain, and that's not what we expected. On the other hand, we can maybe let this in first and plan another refactor.

In addition, since this PR may change the output of AnnotateTaret, you need to add corresponding tests to the follow-up passes (i.e., MergeCompilerRegion and GraphPartition).

@rohanmukh @codeislife99 please help check if this change affect any existing workloads.

cc @zhiics @anijain2305

tests/python/relay/test_pass_annotate_target.py

src/relay/transforms/annotate_target.cc

comaniac · 2020-11-20T21:02:38Z

src/relay/transforms/annotate_target.cc

-      op_expr_to_target_[new_expr] = op_expr_to_target_[expr];
+    const CallNode* call = expr.as<CallNode>();
+    if (op_expr_to_target_.find(expr) != op_expr_to_target_.end()) {
+      // Check whether expr has args, if not - do not insert compiler_end.


Cannot connect this comment to the following logic. Could you elaborate?

The comment related to this part (call && !call->args.empty())) of the condition

comaniac · 2020-11-20T21:11:31Z

src/relay/transforms/annotate_target.cc

+      // Already annotated. Recover target
+      if (op_expr_to_target_.find(input_expr) == op_expr_to_target_.end()) {
+        op_expr_to_target_[input_expr] = post.as<CallNode>()->attrs.as<CompilerAttrs>()->compiler;
+      }


Looks like you don't need the IF? Even input_expr is already in op_expr_to_target_, you can still override it, as suggested by the comment in L188. Accordingly, if you will override the target, you need InsertAnnotation.

If this is not the first invocation of the pass this branch supposed to restore targets from already annotated ops.

comaniac · 2020-11-20T21:14:31Z

src/relay/transforms/annotate_target.cc

+        if (arg_target != default_target) {
+          // annotated already
+          return post;


Looks like you remove the feature that considers the target in existing annotation nodes?

Here it is peeking first arg and if it is already annotated with non-default target it returns the node untouched, preserving the target.

comaniac · 2020-11-20T21:17:48Z

src/relay/transforms/annotate_target.cc

+    supported_targets.push_back(default_target);  // Make default as the last option.
+    // Visit and mutate arguments after the target of this op has been determined.
+    Call post_call = Downcast<Call>(post);
+    if (pre->op->IsInstance<VarNode>()) {


Could you elaborate why a CallNode may have a VarNode as its op?

The test case is tests/python/relay/test_pass_annotate_target.py::test_while_let

zhiics · 2020-11-25T17:22:00Z

src/relay/transforms/annotate_target.cc

+    if (op_expr_to_target_.find(expr) != op_expr_to_target_.end()) {
+      // Check whether expr has args, if not - do not insert compiler_end.
+      if (expr->IsInstance<RefWriteNode>() || expr->IsInstance<RefCreateNode>() ||
+          expr->IsInstance<RefReadNode>() || expr->IsInstance<TupleNode>() ||


There would be more nodes, like constructors. But I am still concerned if this changed is needed. This really makes this already complicated pass more complicated. I still don't see a good point why we don't run mergecompilerregions. That would solve this problem. Without running it, we would have a large number of small segments, which requires frequent data transfer between the host and device as well as frequent kernel launch.

While running merge compiler regions helps cut down the regions, it also makes the external codegen's responsibility to allocate memory for intermediate tensors on those partitions. Thus, in the specific case of ACL, I think there is not much gained by such a merger as ACL would be implementing each ACL primitive operator and let tvm handle the memory allocation of the tensors passed onto external function. Moreover, the kernel launch overhead should also be minimal as it is running on the CPU (so the host and device is almost the same here). Also such a merger will also make the IO tensors live throughout the execution of external function while the space could be re-used if it was not merged.

The problem is the specification of the ACL did not indicate the simple regions (or non-call ops) to be annotated, thus annotate target here is doing something extra than it was asked.

I quite agree that this pass is complicated and needs breakdown. I guess that should be discussed in a RFC as to how it should look like. One direction maybe to take out the annotation of simple regions (non-call ops) as a seperate part ( I believe this was how it looked liked sometime back when it had something called AnnotateRestDefault until it got merged here :) ).

See my comment here : #5277

yeah, it would be nice to have the RFC and list the options there

d-smirnov · 2020-12-09T11:45:53Z

@comaniac Ping. How can we make some progress here?

comaniac · 2020-12-09T19:13:47Z

@comaniac Ping. How can we make some progress here?

I still don't think we should make AnnotateTarget pass even more complicate, so I proposed to have a separate ACL specific pass to workaround this issue like TensorRT. Later we could have an RFC to aggregate those passes and make a single, general pass for all BOYC targets.

cc @zhiics @mbaret @manupa-arm for comments.

mbaret · 2020-12-10T14:01:52Z

I don't agree that this should be separate, because the current behaviour of AnnotateTarget is simply incorrect. ACL does not support tuples, so AnnotateTarget should not mark them as supported. We shouldn't need a 'fix-up' pass after AnnotateTarget to make it correct, it's just a bug in AnnotateTarget that's come about from faulty reasoning about how codegens work. As this is user-facing and a critical error, I think the priority is accepting a fix with a refactor to reduce complexity coming later.

comaniac

Per offline discussion, we will let this PR in first. Afterwards, @mbaret will send an RFC discussing the desire behavior of AnnotateTarget.

@mbaret please merge this PR if that works for you. Thanks.

mbaret

LGTM, but yes we need to specify the expected behaviour of this pass more formally. We'll follow up with an RFC to this effect in the new year.

mbaret · 2020-12-17T09:58:29Z

Thanks everyone.

…pache#6655) * [BYOC] Added annotate_non_call_ops parameter to AnnotateTarget pass Added annotate_non_call_ops parameter to AnnotateTarget pass to prevent non-call to be promoted to previously annotated operations This is useful in case if you are not running MergeCompilerRegions pass after AnnotateTarget. * linter * Tuple and TupleGetItem handling * resored transform.py, added missing tests to main * requested changes

comaniac requested review from zhiics and comaniac October 9, 2020 19:39

tqchen changed the base branch from master to main October 11, 2020 18:16

manupak requested changes Oct 12, 2020

View reviewed changes

trevor-m reviewed Oct 12, 2020

View reviewed changes

python/tvm/relay/transform/transform.py Outdated Show resolved Hide resolved

mbaret requested changes Oct 13, 2020

View reviewed changes

tests/python/relay/test_pass_annotate_target.py Outdated Show resolved Hide resolved

tests/python/relay/test_pass_annotate_target.py Outdated Show resolved Hide resolved

zhiics reviewed Oct 13, 2020

View reviewed changes

python/tvm/relay/transform/transform.py Outdated Show resolved Hide resolved

d-smirnov force-pushed the bugfix-segv branch from f22bf74 to 301bd38 Compare November 19, 2020 22:26

d-smirnov added 3 commits November 19, 2020 22:42

linter

7f66d83

Tuple and TupleGetItem handling

093b70a

resored transform.py, added missing tests to main

b3052cc

d-smirnov requested review from mbaret and zhiics November 20, 2020 20:33

d-smirnov changed the title ~~[BYOC] Added default_tuples parameter to AnnotateTarget pass~~ [BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass Nov 20, 2020

comaniac requested changes Nov 20, 2020

View reviewed changes

requested changes

6948760

zhiics reviewed Nov 25, 2020

View reviewed changes

comaniac approved these changes Dec 15, 2020

View reviewed changes

mbaret approved these changes Dec 17, 2020

View reviewed changes

mbaret merged commit 4060b4f into apache:main Dec 17, 2020

junrushao mentioned this pull request Nov 1, 2021

Apache TVM v0.8 Release Note Candidate #9416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass #6655

[BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass #6655

d-smirnov commented Oct 9, 2020 •

edited

Loading

comaniac commented Oct 9, 2020

manupak left a comment

comaniac commented Oct 13, 2020

d-smirnov commented Nov 15, 2020

trevor-m commented Nov 15, 2020

d-smirnov commented Nov 15, 2020 •

edited

Loading

d-smirnov commented Nov 20, 2020

comaniac left a comment

comaniac Nov 20, 2020

d-smirnov Nov 23, 2020

comaniac Nov 20, 2020

d-smirnov Nov 23, 2020 •

edited

Loading

comaniac Nov 20, 2020

d-smirnov Nov 23, 2020

comaniac Nov 20, 2020

d-smirnov Nov 23, 2020 •

edited

Loading

zhiics Nov 25, 2020

manupak Nov 25, 2020 •

edited

Loading

manupak Nov 25, 2020

zhiics Nov 30, 2020

d-smirnov commented Dec 9, 2020

comaniac commented Dec 9, 2020

mbaret commented Dec 10, 2020

comaniac left a comment •

edited

Loading

mbaret left a comment

mbaret commented Dec 17, 2020

[BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass #6655

[BYOC] Added "include_non_call_ops" parameter to AnnotateTarget pass #6655

Conversation

d-smirnov commented Oct 9, 2020 • edited Loading

comaniac commented Oct 9, 2020

manupak left a comment

Choose a reason for hiding this comment

comaniac commented Oct 13, 2020

d-smirnov commented Nov 15, 2020

trevor-m commented Nov 15, 2020

d-smirnov commented Nov 15, 2020 • edited Loading

d-smirnov commented Nov 20, 2020

comaniac left a 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

d-smirnov Nov 23, 2020 • 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

d-smirnov Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manupak Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-smirnov commented Dec 9, 2020

comaniac commented Dec 9, 2020

mbaret commented Dec 10, 2020

comaniac left a comment • edited Loading

Choose a reason for hiding this comment

mbaret left a comment

Choose a reason for hiding this comment

mbaret commented Dec 17, 2020

d-smirnov commented Oct 9, 2020 •

edited

Loading

d-smirnov commented Nov 15, 2020 •

edited

Loading

d-smirnov Nov 23, 2020 •

edited

Loading

d-smirnov Nov 23, 2020 •

edited

Loading

manupak Nov 25, 2020 •

edited

Loading

comaniac left a comment •

edited

Loading