Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TOPI] Alleviate hanging issue caused by concat op #3268

Closed
wants to merge 2 commits into from

Conversation

@Laurawly
Copy link
Member

commented May 31, 2019

@tqchen This is a temporary solution to alleviate the problem, will work on the IR builder to write the schedule for concate op.

@tqchen

This comment has been minimized.

Copy link
Member

commented May 31, 2019

cc @jroesch @wweic the CI problem could relate to limitations in VM

@masahi

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

operator fusion tests are heavily dependent on concat being fusible as injective. If we want to make concat opaque, corresponding updates to fusion tests are needed.

If this is indeed a temporary solution, I suggest just disable fusion tests that involve concat.

@jroesch

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

I have a patch for the VM issue which is almost complete, will post tomorrow sometime.

@kevinthesun

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2019

Will this change affect the performance of concat for cpu?

@Laurawly

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2019

Will this change affect the performance of concat for cpu?

Previously ssd_mobilenet1.0_512 cost 1.4437s on arm CPU now it costs 1.4545s.

@kevinthesun

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

I tested on x86 cpu with a set of gluoncv models. This change doesn't affect the performance.

@apivovarov

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

This fix solves 10-15 min hanging issue for the first inference
But it causes 20% performance degradation for the second and consequent inferences on GPU.
I tested it for GluonCV ssd_512_mobilenet1.0_voc model on Mali GPU OpenCL (RK3399)
The second and consequent inferences take:
before the fix: 420ms
after the fix: 520ms

@Laurawly

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

This fix solves 10-15 min hanging issue for the first inference
But it causes 20% performance degradation for the second and consequent inferences on GPU.
I tested it for GluonCV ssd_512_mobilenet1.0_voc model on Mali GPU OpenCL (RK3399)
The second and consequent inferences take:
before the fix: 420ms
after the fix: 520ms

The performance reported by tvm evaluator with hanging issue was 470 ms on Mali GPU RK3399. But the hanging time is more than 10 minutes which composes most of the end-to-end time. And by this fix, the hanging issue is alleviated, and performance evaluated by tvm time evaluator is 517 ms. You are more than welcome to fix the hanging issue without performance lost.

@Laurawly Laurawly force-pushed the Laurawly:dev branch from 5302d8d to b029db3 Jun 6, 2019

@Laurawly

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

operator fusion tests are heavily dependent on concat being fusible as injective. If we want to make concat opaque, corresponding updates to fusion tests are needed.

If this is indeed a temporary solution, I suggest just disable fusion tests that involve concat.

@tqchen Shall we disable fusion tests that involve concat?

@hlu1

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

@apivovarov, making Concat opaque allows you to write fast schedules specially for concat, for instance, https://github.com/dmlc/tvm/blob/master/topi/python/topi/x86/injective.py#L53 for x86 concat schedules. Note that the concat schedule is only guaranteed to be used only when concat is an opaque or output_elem_fusable op. The default injective schedule would be used when concat is an injective op and fused together with other injective ops.

@tqchen

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

@Laurawly Laurawly force-pushed the Laurawly:dev branch from b029db3 to e041a90 Sep 5, 2019

@tqchen

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

Close this for now due to inactive status, @sxjscience has volunteered to continue working on the thread https://discuss.tvm.ai/t/explore-optimizations-for-concat/2435/7

@sxjscience

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

@tqchen I'm still working on that. Need to wait for some more time...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.