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

Use number of specified thread for the compilation #2978

Merged
merged 2 commits into from
Apr 17, 2024
Merged

Conversation

umangyadav
Copy link
Member

@umangyadav umangyadav commented Apr 17, 2024

Currently MIGRAPHX_GPU_COMPILE_PARALLEL has no effect because par_for.hpp is not passing min_grain param. This PR fixes that.

@umangyadav umangyadav requested a review from causten as a code owner April 17, 2024 15:18
@umangyadav umangyadav requested review from pfultz2 and TedThemistokleous and removed request for causten April 17, 2024 15:18
@umangyadav umangyadav self-assigned this Apr 17, 2024
Copy link
Collaborator

@TedThemistokleous TedThemistokleous left a comment

Choose a reason for hiding this comment

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

LGTM. @causten this may be related to that issue we saw last week with par_for falling especially that stack dump we saw in within simple_par_for in gdb.

@TedThemistokleous TedThemistokleous added the bugfix Fixes a bug found in the code. label Apr 17, 2024
@pfultz2
Copy link
Collaborator

pfultz2 commented Apr 17, 2024

We shouldn't do this. The min_grain will be ignored when we are using ParallelSTL. Instead, we should use simple_par_for directly to ensure it will always create the correct number of threads. We could probably have par_for with min_grain call simple_par_for directly and the overload without the min_grain can use par_for_each.

@umangyadav
Copy link
Member Author

We shouldn't do this. The min_grain will be ignored when we are using ParallelSTL. Instead, we should use simple_par_for directly to ensure it will always create the correct number of threads. We could probably have par_for with min_grain call simple_par_for directly and the overload without the min_grain can use par_for_each.

Done

@TedThemistokleous
Copy link
Collaborator

We shouldn't do this. The min_grain will be ignored when we are using ParallelSTL. Instead, we should use simple_par_for directly to ensure it will always create threads. We could probably have par_for with min_grain call simple_par_for directly and the overload without the min_grain can use par_for_each.

Ah, that makes better sense and good catch. Didn't really think about the parallel STL case.

Copy link

codecov bot commented Apr 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.86%. Comparing base (b49d539) to head (5e3082a).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #2978   +/-   ##
========================================
  Coverage    91.86%   91.86%           
========================================
  Files          484      484           
  Lines        18609    18609           
========================================
  Hits         17095    17095           
  Misses        1514     1514           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@causten causten merged commit 746390b into develop Apr 17, 2024
47 of 48 checks passed
@causten causten deleted the fix_parallel branch April 17, 2024 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a bug found in the code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants