Skip to content

Conversation

jsjodin
Copy link

@jsjodin jsjodin commented Sep 17, 2024

This patch moves the reduction lowering from the DistributeOp to the TeamsOp by enabling the reduction clauses on the TeamsOp and generate the reductions from those clauses. This makes the composite code generation obsolete since the reduction information can be recomputed. This change should also allow having separate teams and wsloop reductions in the same block.
Co-author: @skatrak

Copy link
Member

@skatrak skatrak left a comment

Choose a reason for hiding this comment

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

Thank you Jan, this indeed looks a lot better. I have a couple of nits, but generally LGTM. If it's already passing Lit and smoke-fort tests I think we should merge it.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Maybe we can name this test "reduction-target-spmd.f90" and keep another simpler "reduction-teams.f90" with the same example that was removed from the "Todo" subdirectory.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Remove HasDistribute function argument, since it's no longer necessary.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that's a good point.

Comment on lines 928 to 929
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bool isNowait = false, bool isTeamsReduction = false,
bool hasDistribute = false) {
bool isNowait = false, bool isTeamsReduction = false) {

Comment on lines 3833 to 3834
Copy link
Member

Choose a reason for hiding this comment

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

Nit: We can remove this comment now, since we're no longer looking for the wrappers of an omp.loop_nest.

Copy link
Member

Choose a reason for hiding this comment

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

Don't we currently need this for other reductions? If not, do we expect to use it elsewhere or is this actually only needed to be calculated for teams reductions within a target region?

Copy link
Author

Choose a reason for hiding this comment

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

I think it is only needed for the team reduction, but I figured if we ever want to know the data size for parallel/workshare it doesn't hurt to parameterize the code.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think the previous formatting is the preferred one (no unnecessary braces and no 'else' after 'return').

@jsjodin jsjodin force-pushed the jleyonberg/teams-reduction branch from 3861586 to 2287513 Compare September 30, 2024 22:11
@jsjodin jsjodin merged commit 66ee96f into amd-trunk-dev Sep 30, 2024
2 of 4 checks passed
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.

2 participants