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

[USMP] Update RFC with constants pools #81

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

manupak
Copy link
Contributor

@manupak manupak commented Jun 23, 2022

This commit introduces the notion of constant memory pools
and removes the need to define access on each of the targets.

This commit introduces the notion of constant memory pools
and removes the need to define access on each of the targets.

Change-Id: I96822cd905716fc13efa6fe58d13bcf7a9da8dee
@manupak
Copy link
Contributor Author

manupak commented Jun 23, 2022

cc : @areusch

Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

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

thanks @manupa-arm , just a question here which i might have missed before

```
The input IRModule is expected to have "candidate_memory_pools" annotation populated with a orderered list of PoolInfo objects. The ordering will indicate to the planner the order of preference each allocate will be pinned to each Pool. The core compiler will run a pass to assign each tir.allocate with candidate_memory_pools based on the target each PrimFunc is executed, prior to invoking the USMP.
The input IRModule is expected to have "candidate_memory_pools" annotation populated with a orderered list of WorkspacePoolInfo objects. The ordering will indicate to the planner the order of preference each allocate will be pinned to each Pool. The core compiler will run a pass to assign each tir.allocate with candidate_memory_pools based on the target each PrimFunc is executed, prior to invoking the USMP.
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if we should wade into the ordering/prioritization problem here. i mean we could, but it does seem like a bit of a can of worms. we could also solve this by customizing the allocator. just curious if you have a use case in mind?

Copy link
Contributor Author

@manupak manupak Jul 1, 2022

Choose a reason for hiding this comment

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

thanks @areusch for taking a look.

The ordering/prioritization is not in the scope of this change.

In fact, it is something already supported :

https://github.com/apache/tvm/blob/395e91ff54543864a90240d18c8efd8c277c758b/tests/python/unittest/test_tir_usmp_algo.py#L366-L413

However, the USMP will only try to solve the allocation problem not the multi-objective problem of memory and performance. In other words, the scheduling stage is responsible for assigning the right pool for each allocate node. However, if the scheduling does not care about assigning a singular pool (i.e. scheduling is fine with a set of pools), USMP will try to do a prioritized/ordered consumption of the pools to assign all allocated nodes to memory pools.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, that makes sense to me

Copy link
Contributor

Choose a reason for hiding this comment

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

only request--shall we document the above somewhere? maybe in a file-level docstring for USMP?

Copy link
Contributor Author

@manupak manupak Jul 12, 2022

Choose a reason for hiding this comment

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

Sure, happy to do that in a follow up -- I ll add it to the main pass.

@areusch
Copy link
Contributor

areusch commented Jul 12, 2022

i have no concerns here, @manupa-arm feel free to submit this when you're ready to start

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants