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.

Sign up for GitHub

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

Jump to bottom

[USMP] Hill Climb allocator #9704

Merged
merged 5 commits into from
Jan 14, 2022
Merged

[USMP] Hill Climb allocator #9704

merged 5 commits into from
Jan 14, 2022

Conversation

d-smirnov
Copy link
Contributor

This PR adds HillClimb allocator "tir.usmp.algo.hill_climb"
to memory allocation algorithm set

manupak
manupak requested changes Dec 13, 2021
View reviewed changes
Copy link
Contributor

@manupak manupak left a comment

Choose a reason for hiding this comment

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

Thanks @d-smirnov.

I did a pass.

Most of my comments are on cleaning up the code, improving clarity and possibly re-using the code rather than duplicating it here.

See if you agree.

src/tir/usmp/algo/hill_climb.cc Outdated Show resolved Hide resolved
src/tir/usmp/algo/hill_climb.cc Outdated Show resolved Hide resolved
src/tir/usmp/algo/hill_climb.cc Outdated Show resolved Hide resolved
src/tir/usmp/algo/hill_climb.cc Outdated Show resolved Hide resolved
src/tir/usmp/algo/hill_climb.cc Outdated Show resolved Hide resolved
tests/python/unittest/test_tir_usmp_algo_hill_climb.py Outdated Show resolved Hide resolved
tests/python/unittest/test_tir_usmp_algo_hill_climb.py Show resolved Hide resolved
tests/python/unittest/test_tir_usmp_algo_hill_climb.py Show resolved Hide resolved
src/tir/usmp/algo/hill_climb.cc Show resolved Hide resolved
src/tir/usmp/algo/hill_climb.cc Outdated Show resolved Hide resolved
@manupak
Copy link
Contributor

manupak commented Dec 14, 2021

Should we merge the testing here into https://github.com/apache/tvm/blob/af869bc79cecdc18371862524ea241803dc78c7a/tests/python/unittest/test_tir_usmp_algo.py ?

By doing so, we could improve the current testing scope to consider hill climbing allocation while augmenting the tests here to run other algorithms as well, because the tests here seems not special just only to hill climbing algorithm.

@d-smirnov
[USMP] Hill Climb allocator
f2423d5 
This PR adds HillClimb allocator "tir.usmp.algo.hill_climb"
to the memory allocation algorithm set.

Change-Id: Ib7485df93757eb512da040528ec86c920db8d03b
@d-smirnov
requested changes
2b16728 
Change-Id: I6700a24c1608d92f87be7dde33cc24f5de1f7063
@d-smirnov
Conda-related linter small fixes
eb72b02 
Change-Id: I0dac5c6d75ade8f813b077c8708aad59d2722933
manupak
manupak requested changes Dec 20, 2021
View reviewed changes
Copy link
Contributor

@manupak manupak left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looking good now... Thanks @d-smirnov

Some early feedback for the header file creation.

It is better if we can enable hill climb in other tests in test_tir_usmp_algo.py as well.

src/tir/usmp/algo/greedy.cc Show resolved Hide resolved
src/tir/usmp/algo/hill_climb.cc Show resolved Hide resolved
src/tir/usmp/algo/hill_climb.cc Show resolved Hide resolved
@d-smirnov
Moved implementation from greedy.h to greedy.cc
7b4dceb 
Change-Id: If8ed159eceef32d3f22b51e0252161d09222eb1e
manupak
manupak reviewed Jan 11, 2022
View reviewed changes
Copy link
Contributor

@manupak manupak left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @d-smirnov,

Thanks for working with me on this. This looks good now!
One last request is to enable hill_climb in the tests in test_tir_usmp_algo.py that are parameterized on greedy algorithm. Would we able to do that ?

e.g. :

tvm/tests/python/unittest/test_tir_usmp_algo.py

Lines 126 to 127 in 46b3fa7

@pytest.mark.parametrize("algorithm", ["greedy_by_size", "greedy_by_conflicts"])
def test_name_based_ordering(algorithm):

Also, lets just re-trigger the CI once more since this is 19 days old.

@d-smirnov
Integrated into test_tir_usmp_algo.py unit test
558eac8 
Added "hill_climb" into test_tir_usmp_algo.py
Amended sorting to be consistent with "greedy" family

Change-Id: I8e9f5282f15baaab71d6d129aeb9643376b14763
manupak
manupak approved these changes Jan 14, 2022
View reviewed changes
Copy link
Contributor

@manupak manupak left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @d-smirnov

@manupak manupak merged commit 89ae603 into apache:main Jan 14, 2022
@manupak manupak mentioned this pull request Jan 20, 2022
Closed
21 tasks
crazydemo pushed a commit to crazydemo/tvm that referenced this pull request Jan 27, 2022
@d-smirnov @crazydemo
[USMP] Hill Climb allocator (apache#9704)
62264e3 
* [USMP] Hill Climb allocator

This PR adds HillClimb allocator "tir.usmp.algo.hill_climb"
to the memory allocation algorithm set.

Change-Id: Ib7485df93757eb512da040528ec86c920db8d03b

* requested changes

Change-Id: I6700a24c1608d92f87be7dde33cc24f5de1f7063

* Conda-related linter small fixes

Change-Id: I0dac5c6d75ade8f813b077c8708aad59d2722933

* Moved implementation from greedy.h to greedy.cc

Change-Id: If8ed159eceef32d3f22b51e0252161d09222eb1e

* Integrated into test_tir_usmp_algo.py unit test

Added "hill_climb" into test_tir_usmp_algo.py
Amended sorting to be consistent with "greedy" family

Change-Id: I8e9f5282f15baaab71d6d129aeb9643376b14763
Raghav-Chakravarthy pushed a commit to Raghav-Chakravarthy/tvm that referenced this pull request Jan 28, 2022
@d-smirnov @Raghav-Chakravarthy
[USMP] Hill Climb allocator (apache#9704)
711c7d6 
* [USMP] Hill Climb allocator

This PR adds HillClimb allocator "tir.usmp.algo.hill_climb"
to the memory allocation algorithm set.

Change-Id: Ib7485df93757eb512da040528ec86c920db8d03b

* requested changes

Change-Id: I6700a24c1608d92f87be7dde33cc24f5de1f7063

* Conda-related linter small fixes

Change-Id: I0dac5c6d75ade8f813b077c8708aad59d2722933

* Moved implementation from greedy.h to greedy.cc

Change-Id: If8ed159eceef32d3f22b51e0252161d09222eb1e

* Integrated into test_tir_usmp_algo.py unit test

Added "hill_climb" into test_tir_usmp_algo.py
Amended sorting to be consistent with "greedy" family

Change-Id: I8e9f5282f15baaab71d6d129aeb9643376b14763
@PhilippvK PhilippvK mentioned this pull request Feb 7, 2022
Merged
ylc pushed a commit to ylc/tvm that referenced this pull request Feb 16, 2022
@d-smirnov @ylc
[USMP] Hill Climb allocator (apache#9704)
0436089 
* [USMP] Hill Climb allocator

This PR adds HillClimb allocator "tir.usmp.algo.hill_climb"
to the memory allocation algorithm set.

Change-Id: Ib7485df93757eb512da040528ec86c920db8d03b

* requested changes

Change-Id: I6700a24c1608d92f87be7dde33cc24f5de1f7063

* Conda-related linter small fixes

Change-Id: I0dac5c6d75ade8f813b077c8708aad59d2722933

* Moved implementation from greedy.h to greedy.cc

Change-Id: If8ed159eceef32d3f22b51e0252161d09222eb1e

* Integrated into test_tir_usmp_algo.py unit test

Added "hill_climb" into test_tir_usmp_algo.py
Amended sorting to be consistent with "greedy" family

Change-Id: I8e9f5282f15baaab71d6d129aeb9643376b14763
@driazati driazati mentioned this pull request Jul 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manupak manupak manupak approved these changes

@areusch areusch Awaiting requested review from areusch

@comaniac comaniac Awaiting requested review from comaniac

@Hzfengsy Hzfengsy Awaiting requested review from Hzfengsy

@icemelon icemelon Awaiting requested review from icemelon

@jroesch jroesch Awaiting requested review from jroesch

@junrushao junrushao Awaiting requested review from junrushao

@kparzysz-quic kparzysz-quic Awaiting requested review from kparzysz-quic

@masahi masahi Awaiting requested review from masahi

@merrymercy merrymercy Awaiting requested review from merrymercy

@tqchen tqchen Awaiting requested review from tqchen

@vinx13 vinx13 Awaiting requested review from vinx13

@yzhliu yzhliu Awaiting requested review from yzhliu

@ZihengJiang ZihengJiang Awaiting requested review from ZihengJiang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@d-smirnov @manupak