Skip to content

[WIP][Runtime]Pipeline Executor For Compute graph pipeline - #7892

Closed
huajsj wants to merge 28 commits into
apache:mainfrom
huajsj:main
Closed

[WIP][Runtime]Pipeline Executor For Compute graph pipeline#7892
huajsj wants to merge 28 commits into
apache:mainfrom
huajsj:main

Conversation

@huajsj

@huajsjhuajsj commented Apr 20, 2021

Copy link
Copy Markdown
Contributor

Issue:
SOC hardware plarform have multiple types compute chipset like
GPU,FPGA,APU,RPU etc, there is a requirement that use these compute
unit in parallel to reach best performance.

Solution:
In these pipeline solution, we first split the compute graph into
a group of subgraph, then run these subgraph in a pipeline module
to make the GPU/FPGA/APU/RPU parallel running become possible.

Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers by @ them in the pull request thread.

@huajsjhuajsj changed the title Pipeline Compute Graph With New Subgraph Executor[Runtime]Pipeline Compute Graph With New Subgraph ExecutorApr 21, 2021
@tqchen

tqchen commented Apr 26, 2021

Copy link
Copy Markdown
Member

cc @comaniac@areusch@tmoreau89

@huajsj

huajsj commented Apr 26, 2021

Copy link
Copy Markdown
ContributorAuthor

Thanks @tqchen for the follow up, I proposed a RFC(https://discuss.tvm.apache.org/t/rfc-compute-graph-pipeline-with-new-subgraph-executor/9839) to explain the motivation and solution architecture as reference.

@huajsjhuajsj changed the title [Runtime]Pipeline Compute Graph With New Subgraph Executor[WIP][Runtime]Pipeline Compute Graph With New Subgraph ExecutorApr 28, 2021
@huajsjhuajsj changed the title [WIP][Runtime]Pipeline Compute Graph With New Subgraph Executor[Runtime]Pipeline Executor For Compute graph pipelineMay 1, 2021
comaniac
comaniac requested changes May 4, 2021
View reviewed changes

@comaniaccomaniac left a comment

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.

Reviewed the test for APIs and user interfaces, but it needs lots of changes so I'll stop here for now.

Comment threadtests/python/relay/test_analysis_pipeline.py Outdated
Comment threadtests/python/relay/test_analysis_pipeline.py Outdated
Comment threadtests/python/relay/test_analysis_pipeline.py Outdated
Comment threadtests/python/relay/test_analysis_pipeline.py Outdated
Comment threadtests/python/relay/test_analysis_pipeline.py Outdated
Comment threadtests/python/relay/test_analysis_pipeline.py Outdated
Comment threadtests/python/relay/test_analysis_pipeline.py Outdated
Comment threadtests/python/relay/test_analysis_pipeline.py Outdated
Comment threadtests/python/relay/test_analysis_pipeline.py Outdated
Comment threadtests/python/relay/test_analysis_pipeline.py Outdated
@huajsj
huajsj requested a review from comaniacJune 1, 2021 18:42
@huajsj

huajsj commented Jun 1, 2021

Copy link
Copy Markdown
ContributorAuthor

@comaniac@areusch@tmoreau89

comaniac
comaniac requested changes Jun 1, 2021
View reviewed changes

@comaniaccomaniac left a comment

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.

Alco cc @areusch@tmoreau89

Comment threadcmake/config.cmake Outdated
Comment threadpython/tvm/contrib/pipeline_executor.py Outdated
Comment threadpython/tvm/contrib/pipeline_executor.py Outdated
Comment threadpython/tvm/contrib/pipeline_executor.py Outdated
Comment threadpython/tvm/contrib/pipeline_executor.py Outdated
Comment threadpython/tvm/contrib/pipeline_executor.py
Comment threadpython/tvm/contrib/pipeline_executor.py Outdated
Comment threadpython/tvm/contrib/pipeline_executor.py Outdated
params : dict of str to NDArray
Additional arguments
"""
if key is not None:

@comaniaccomaniacJun 1, 2021

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.

Why key and value are allowed to be None?

@huajsjhuajsjJun 4, 2021

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.

fixed.

@comaniaccomaniacJun 22, 2021

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.

Didn't see the fix? Why we need if key is not None?

Comment threadsrc/runtime/pipeline/pipeline_executor.cc Outdated
Comment threadsrc/runtime/pipeline/pipeline_executor.cc Outdated
@huajsj
huajsjforce-pushed the main branch 4 times, most recently from 6061ae2 to 1c67a87CompareJune 16, 2021 21:52
@huajsj
huajsj requested a review from comaniacJune 16, 2021 23:01
@huajsj

huajsj commented Jun 17, 2021

Copy link
Copy Markdown
ContributorAuthor

@comaniac

@comaniac

comaniac commented Jun 17, 2021

Copy link
Copy Markdown
Contributor

@comaniac

I'm busy with other tasks in recent days. Will try to take another look when I got a chance.

comaniac
comaniac requested changes Jun 23, 2021
View reviewed changes
Comment threadcmake/config.cmake
Comment threadpython/tvm/contrib/graph_executor.py Outdated
Comment threadpython/tvm/contrib/pipeline_executor.py
Comment threadpython/tvm/contrib/pipeline_executor.py Outdated
Comment threadpython/tvm/contrib/pipeline_executor.py Outdated
Comment threadsrc/runtime/pipeline/pipeline_executor.h Outdated
Comment threadsrc/runtime/pipeline/pipeline_executor.cc Outdated
Comment threadsrc/runtime/pipeline/pipeline_executor.cc Outdated
Comment threadsrc/runtime/pipeline/pipeline_executor.cc Outdated
Comment threadsrc/runtime/pipeline/pipeline_executor.h
@huajsj
huajsjforce-pushed the main branch 2 times, most recently from 2947327 to 62e4d5fCompareJune 30, 2021 06:17
@huajsj
huajsj requested a review from comaniacJuly 6, 2021 18:53
@huajsj

huajsj commented Jul 7, 2021

Copy link
Copy Markdown
ContributorAuthor

@comaniac

@huajsj

huajsj commented Jul 9, 2021

Copy link
Copy Markdown
ContributorAuthor

@comaniac, if you have time , could you help for a review. thanks.

@huajsjhuajsj mentioned this pull request Jul 29, 2021
Closed
15 tasks
masahi
masahi reviewed Sep 3, 2021
View reviewed changes
Comment threadsrc/runtime/pipeline/pipeline_data.h

template <typename SLOT_TYPE = SLOT>
void deleteQueue(squeue<SLOT_TYPE>* q) {
free(q);

@masahimasahiSep 3, 2021

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.

use delete

masahi
masahi reviewed Sep 3, 2021
View reviewed changes
Comment threadsrc/runtime/pipeline/pipeline_struct.h
#include <assert.h>
#include <sched.h>
#include <string.h>
#include <sys/syscall.h>

@masahimasahiSep 3, 2021

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.

Only compiles on linux

@masahi

masahi commented Sep 3, 2021•
edited
Loading

Copy link
Copy Markdown
Member

Thanks @huajsj. Please respect our coding style and standard good programming practices:

  • For C++ code: Variable names should be snake_case and function / class names should be CamelCase
  • Do not use using namespace without good reason
  • Do not introduce complicated typedef like typedef unordered_map<int, unordered_map<int, unordered_map<int, string>>> PIPELINE_CONF;. Introducing a class should be more readable.
  • Do not use weird type name like RUNTIME_PIPELINE_OUTPUT_CONF.

@huajsj
Add network split logic into test.
47c5cc2
@huajsj
change moudle index into start 0.
b42de5a
@huajsj
fix mod_idx = 0 crash issue, but still logic wrong
25faf19
@huajsj
fix crash issue.
2e0c1c3
@huajsj
fix input data not get correct set issue.
fba531e
[Finding]
the final output is same with constant value, seems like input data is
0, this is because the get input index have a '+1' operation, but the
input index already start from 0. the means when doing setinput('x',..) it
should get convert to setinput(0, ..), but the wrong logic is
setinput(1,..).
@huajsj
fix set param report error issue.
1ee5831
@comaniac

comaniac commented Nov 2, 2021

Copy link
Copy Markdown
Contributor

IIUC, this PR should be out-of-date? Should we close?

@huajsj

huajsj commented Nov 5, 2021

Copy link
Copy Markdown
ContributorAuthor

@comaniac , yes we should close this PR, closed it now.

@huajsjhuajsj closed this Nov 5, 2021
@huajsjhuajsj mentioned this pull request Feb 1, 2022
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@masahimasahimasahi left review comments
@comaniaccomaniacAwaiting requested review from comaniac
@areuschareuschAwaiting requested review from areusch
@jroeschjroeschAwaiting requested review from jroesch
@junrushaojunrushaoAwaiting requested review from junrushao
@kazumkazumAwaiting requested review from kazum
@liangfuliangfuAwaiting requested review from liangfu
@merrymercymerrymercyAwaiting requested review from merrymercy
@tmoreau89tmoreau89Awaiting requested review from tmoreau89
@tqchentqchenAwaiting requested review from tqchen
@vinx13vinx13Awaiting requested review from vinx13
@yzhliuyzhliuAwaiting requested review from yzhliu
@zhiicszhiicsAwaiting requested review from zhiics
@ZihengJiangZihengJiangAwaiting requested review from ZihengJiang
+1 more reviewer
@HungYangChangHungYangChangHungYangChang approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status: need review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@huajsj@tqchen@comaniac@masahi@HungYangChang