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

split inner reduction heuristics into 2d and 3d heuristics #3330

Merged
merged 4 commits into from
Nov 8, 2024

Conversation

liqiangxl
Copy link
Collaborator

@liqiangxl liqiangxl commented Nov 2, 2024

What's in this PR?
This PR separates inner reduction heuristics into distinct 2D and 3D heuristic functions.

Why?
The 2D and 3D reductions represent different domain structures within the reduction tensor view:

  • 2D inner reduction: Domains in the reduction tensor view can be merged into a 2D structure, [I, R].
  • 3D inner reduction: Domains can be merged into a 3D structure, [R, I, R].

These two configurations require different parallelization strategies, so keeping them in separate functions enhances maintainability and allows for individual optimization of each heuristic.

code changes
The existing innerReductionHeuristic() is duplicated as inner2dReductionHeuristic and inner3dReductionHeuristic, will clean inner2dReductionHeuristic in a separate PR.

@liqiangxl
Copy link
Collaborator Author

!build

@liqiangxl
Copy link
Collaborator Author

!build

@liqiangxl liqiangxl marked this pull request as ready for review November 6, 2024 13:35
@liqiangxl liqiangxl requested a review from jjsjann123 November 6, 2024 13:35
Copy link
Collaborator

@jjsjann123 jjsjann123 left a comment

Choose a reason for hiding this comment

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

stamp for mechanical changes.

@@ -63,7 +63,7 @@ void reduceProductTo(int64_t& z, int64_t& y, int64_t& x, const int64_t max) {
}
}

std::unique_ptr<ReductionParams> innerReductionHeuristic(
std::unique_ptr<ReductionParams> inner2dReductionHeuristic(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: this is renamed as a 2d reduction. But in the code comment, we refer to this as 1D schedule, which is a bit confusing.

debug() << rparams->toString() << std::endl;
}

// If 3d, check if it's supported by the scheduler, otherwise force 1D
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh I think I was mistaken. So if we do return rparams at line 947, that's the 1d schedule this comment is referring to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be 2D, we don't have 1D scheduler.

@liqiangxl
Copy link
Collaborator Author

!build

@liqiangxl liqiangxl merged commit 7b3227b into main Nov 8, 2024
11 of 12 checks passed
@liqiangxl liqiangxl deleted the llu/2d_inner_reduction_heuristics branch November 8, 2024 14:21

} else {
rparams->grid_dim_iter_dom = ParallelType::BIDx;
if (gdimx > scheduler_utils::x_grid_limit) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't gdimx set to LaunchParams::UNINITIALIZED_VAL here?
Therefore, this if-block is alway False.

Copy link
Collaborator

Choose a reason for hiding this comment

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

🤕 Is this supposed to be godim? And since this is just a copy of the old function, we also need to fix the other one then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤕 Is this supposed to be godim? And since this is just a copy of the old function, we also need to fix the other one then.

@rdspring1 added a fix in #3432 (comment)

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.

3 participants