Skip to content

dicrease the complexity of CalcDep from exponential to linear#4053

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

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

Conversation

@yaochengji
Copy link
Contributor

@yaochengji yaochengji commented Oct 3, 2019 •
edited
Loading

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

@MarisaKirisame please review, thanks!

@MarisaKirisame
Copy link
Contributor

MarisaKirisame commented Oct 3, 2019

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
Contributor Author

yaochengji commented Oct 4, 2019

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
Contributor

MarisaKirisame commented Oct 4, 2019

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

@yaochengji
Copy link
Contributor Author

yaochengji commented Oct 4, 2019 •
edited
Loading

@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
Contributor

MarisaKirisame commented Oct 4, 2019

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

@yaochengji
Copy link
Contributor Author

yaochengji commented Oct 5, 2019

@MarisaKirisame do you think this commit should be merged?

@MarisaKirisame
Copy link
Contributor

MarisaKirisame commented Oct 5, 2019

sure. It will increase the robustness of the DCE.

@vinx13
Copy link
Member

vinx13 commented Oct 6, 2019

Please add a test case

@vinx13 vinx13 added the status: need test case need test cases to cover the change label Oct 6, 2019
@yaochengji
Copy link
Contributor Author

yaochengji commented Oct 6, 2019

@vinx13 Done.

vinx13
vinx13 reviewed Oct 6, 2019
View reviewed changes
src/relay/pass/dead_code.cc
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
Member

@vinx13 vinx13 Oct 6, 2019

Choose a reason for hiding this comment

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

why <=2?

Copy link
Contributor

@MarisaKirisame MarisaKirisame Oct 6, 2019

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
Member

@vinx13 vinx13 Oct 6, 2019

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
Contributor Author

@yaochengji yaochengji Oct 7, 2019

Choose a reason for hiding this comment

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

Done.

@vinx13 vinx13 removed the status: need test case need test cases to cover the change label Oct 6, 2019
@yaochengji yaochengji requested a review from vinx13 October 7, 2019 00:53
@vinx13 vinx13 merged commit 2241272 into apache:master Oct 7, 2019
@vinx13
Copy link
Member

vinx13 commented Oct 7, 2019

Thanks @yaochengji @MarisaKirisame this is merged

anijain2305 pushed a commit to anijain2305/tvm that referenced this pull request Oct 17, 2019
@yaochengji
dicrease the complexity of CalcDep from exponential to linear (apache…
532b3fc 
…#4053)
wweic pushed a commit to neo-ai/tvm that referenced this pull request Oct 18, 2019
@yaochengji @wweic
dicrease the complexity of CalcDep from exponential to linear (apache…
4b1b88f 
…#4053)
petrex pushed a commit to petrex/tvm that referenced this pull request Oct 29, 2019
Merge branch 'master' into benchmark
de18299 
* 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)
  ...
@tqchen tqchen mentioned this pull request Nov 8, 2019
Closed
@ANSHUMAN87 ANSHUMAN87 mentioned this pull request Mar 4, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MarisaKirisame MarisaKirisame MarisaKirisame approved these changes

@vinx13 vinx13 vinx13 approved these changes

Assignees

No one assigned

Labels

status: accepted

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yaochengji @MarisaKirisame @vinx13