-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[Snippets] SplitDimensionM: heuristic update #28180
base: master
Are you sure you want to change the base?
[Snippets] SplitDimensionM: heuristic update #28180
Conversation
205e16c
to
78a43ee
Compare
@a-sidorova @IvanNovoselov could you please take a look? Thanks in advance |
if (divisor >= min_kernel_m) | ||
return std::make_pair(m_dim / divisor, divisor); | ||
const size_t m_kernel = m_dim / divisor; | ||
if (m_kernel >= min_kernel_m) { | ||
best_result.first = divisor; | ||
best_result.second = m_kernel; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear from this code why we try to find maximal divisor? If divisor >= min_kernel_m
we return, but if m_kernel >= min_kernel_m
we continue. Why?
It seems to me that this problem is symmetrical and should be treated accordingly:
if (m_dim % divisor == 0)
=> we can split M, great. There are 2 ways to do that:
a. (m_dim / divisor, divisor)
b. (divisor, m_dim / divisor)- These 2 ways are absolutely identical except for that this always holds:
divisor < (m_dim / divisor)
- It means that one way to split optimally is to start from the max divisor (=sqrt(m_dim)), go downward and return as soon as the parallel work is sufficient:
if(batch_dim * m_dim/divisor >= optimal_parallelism_work_amount)
. This way we'll make sure that both the kernel WA is maximal (since we're going downwards) and the parallel WA is optimal. - Alternatively, we can start from the minimal acceptable divisor, which should be min_kernel_m, go upward and return as soon as (m_dim % divisor == 0). This way we'll guarantee that the kernel work amount is larger than the minimal one (since we started from min_kernel_m) and the parallel work amount is maximal (since
m_dim / divisor
is deceasing). But that's not the case in this particular function, since we want to maximize the kernel WA. - Sometimes these
min_kernel_m
andoptimal_parallelism_work_amount
can be mutually exclusive, so we should think carefully which is more important. I guess that the parallel work amount should be prioritized, so the approach from 3 should be used. - It looks like we try to implement a mix of the above strategies here: we inspect both a and b splits, and return a if the min_kernel WA is achieved and b if parallel WA is satisfied. This may be not consistent in some circumstances, especially when the parallel & kernel WA limitations can't be fulfilled simultaneously.
static std::pair<size_t, size_t> compute_ideal_cases_heuristic(size_t batch_dim, size_t m_dim, size_t optimal_parallelism_work_amount); | ||
/** | ||
* @brief Aggressively splits m_dim to minimize kernel_m in order to reduce waiting time for idle threads at the last parallel loop iteration. | ||
*/ | ||
static std::pair<size_t, size_t> compute_aggressive_heuristic(size_t batch_dim, size_t m_dim, size_t optimal_parallelism_work_amount); | ||
/** | ||
* @brief Conservatively splits m_dim to get the batch in (optimal_parallelism_work_amount, 2 * optimal_parallelism_work_amount) interval | ||
*/ | ||
static std::pair<size_t, size_t> compute_conservative_heuristic(size_t batch_dim, size_t m_dim, size_t optimal_parallelism_work_amount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor comments with respect to the methods' names:
- the functions try to split M dimension into two: batch_m and new_m, so it seems logical to call them smth like
get_splitted_*
orsplit_m_*
orcompute_m_split
etc. Heuristic is used inside this functions to perform the split, so the functions do not compute heuristic, but rather use it to compute smth else (split). - We should try to use more descriptive names instead of
aggressive
orconservative
, because what seems conservative to one may seem aggressive to the other. Possible options include:split_ideally
,split_max_kernel_work
andsplit_max_parallel_work
Details:
Tickets: