Skip to content

dicrease the complexity of CalcDep from exponential to linear - #4053

Merged
vinx13 merged 1 commit into
apache:masterfrom
yaochengji:fix-dce-pass
Oct 7, 2019
Merged

dicrease the complexity of CalcDep from exponential to linear#4053
vinx13 merged 1 commit into
apache:masterfrom
yaochengji:fix-dce-pass

Conversation

@yaochengji

@yaochengjiyaochengji commented Oct 3, 2019

Copy link
Copy Markdown
Contributor

The DCE pass bug is not caused by infinite loop, it is just time consuming.

@MarisaKirisame please review, thanks!

@MarisaKirisame

Copy link
Copy Markdown
Contributor

LGTM.
note that you should not pass in a graph to DCE. We only intend for it to support tree form. In general, all the high level analysis/pass (pointer analysis, partial eval, dead code elimination, ad) all assume the input is in tree form.

@yaochengji

Copy link
Copy Markdown
ContributorAuthor

Why is it assumed a tree form? I can see that the base class ExprVisitor also use the visit_counter_ to make sure all the expr are visited once.

@MarisaKirisame

Copy link
Copy Markdown
Contributor

@yaochengji because what does it mean for a graph form program to have scope and effect?

@yaochengji

yaochengji commented Oct 4, 2019

Copy link
Copy Markdown
ContributorAuthor

@MarisaKirisame but I think since using a visit_counter to guarantee the complexity is a default behavior in ExprVisitor, it should not be dropped in CalcDep .

cc @icemelon9

@MarisaKirisame

Copy link
Copy Markdown
Contributor

you can still has it. I just mean you should not pass graph in.

@yaochengji

Copy link
Copy Markdown
ContributorAuthor

@MarisaKirisame do you think this commit should be merged?

@MarisaKirisame

Copy link
Copy Markdown
Contributor

sure. It will increase the robustness of the DCE.

@vinx13

Copy link
Copy Markdown
Member

Please add a test case

@vinx13vinx13 added the status: need test case need test cases to cover the change label Oct 6, 2019
@yaochengji

Copy link
Copy Markdown
ContributorAuthor

@vinx13 Done.

void VisitExpr(const Expr& e) final {
return ExprFunctor<void(const Expr& e)>::VisitExpr(e);
visit_counter_[e.get()]++;
if (visit_counter_[e.get()] <= 2) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why <=2?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the dce code seprate variable into three parts:
used 0 times (remove)
used 1 times (inline)
used 2 times (dont do anything).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, let's add a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@vinx13vinx13 removed the status: need test case need test cases to cover the change label Oct 6, 2019
@vinx13
vinx13 merged commit 2241272 into apache:masterOct 7, 2019
@vinx13

Copy link
Copy Markdown
Member

Thanks @yaochengji@MarisaKirisame this is merged

anijain2305 pushed a commit to anijain2305/tvm that referenced this pull request Oct 17, 2019
petrex pushed a commit to petrex/tvm that referenced this pull request Oct 29, 2019
* master: (21 commits)
[Fix][VM] Fix VM invoke with set_params (apache#4079)
[QNN] Refactor fixed point multiplication in requantize (apache#4073)
Fix match case in Python-side expr functor (apache#4037)
Hide symbols from dependent libraries if HIDE_PRIVATE_SYMBOLS is ON. (apache#4041)
Add gradient for log-softmax (apache#4069)
[DOC] Fix typos in tutorials (apache#4066)
dicrease the complexity of CalcDep from exponential to linear (apache#4053)
[Relay][AlterOp] Minor refactor. (apache#4064)
[Relay][AlterOp] Improving support for broadcast layout alteration. (apache#4040)
Add parses support for zeros_like tflite operator (apache#4042)
[Bugfix][TF] reset graph after getting tag of savedmodel (apache#4055)
[Relay][VM] Add more passes to VMCompiler (apache#4058)
[Relay][VM] Add autotvm context when compile (apache#4062)
[Bugfix] Fix target host for vm compiler (apache#4057)
[Relay][Training] Add gradient for Crossentropy (apache#3925)
[llvm] switch to use Align for llvm trunk (apache#4051)
[Relay][TopHub] Add switch to disable TopHub download (apache#4015)
[Relay][Op] Add instance norm op (apache#4004)
[QNN][Relay] Calling Dialect passes from inside Relay Build API. (apache#3971)
[RELAY/PASS] Fix the extent for the post_stmt in the loop partition (apache#3734)
...
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@yaochengji@MarisaKirisame@vinx13