Fix view_reduction to work with output variable coming from either private or shared memory #256

bartgol · 2022-10-12T23:38:19Z

Motivation

In case result came from shared memory (e.g., an entry of a view), race conditions caused wrong results. This PR does the same thing that was done in impl::parallel_reduce, namely uses an automatic local var, then copies back in the output var.

Related Issues

Closes Potential problem with ExeSpaceUtils view_reduction and parallel_reduce #254

E3SM Stakeholder Feedback

Might help scream, even though there's already a fix there.

bartgol · 2022-10-12T23:39:10Z

tests/kokkos/kokkos_utils_tests.cpp

-  test_view_reduction<Real,false,true,16,3> (1.0/3.0,2,11);
-  test_view_reduction<Real, true,false,16,3> (1.0/3.0,2,11);
-  test_view_reduction<Real,false,false,16,3> (1.0/3.0,2,11);
+  test_view_reduction<Real, true,true,18,4> (1.0/3.0,2,11);


I have no idea how 3 was not generating a compiler error. We don't allow packs of non-power-of-2 size. I could have looked into why this does not throw an error, but I figured I fixed it and call it a day.

E3SM-Autotester · 2022-10-13T00:02:38Z

Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request.

E3SM-Autotester · 2022-10-13T00:03:18Z

Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects:

Pull Request Auto Testing STARTING (click to expand)

Build Information

Test Name: EKAT_PullRequest_Autotester_Mappy

Build Num: 350
Status: STARTED

Jenkins Parameters

Parameter Name	Value
EKAT_SOURCE_BRANCH	bartgol/fix-view-reduction
EKAT_SOURCE_REPO	https://github.com/E3SM-Project/EKAT
EKAT_SOURCE_SHA	`4c8d649`
EKAT_TARGET_BRANCH	master
EKAT_TARGET_REPO	https://github.com/E3SM-Project/EKAT
EKAT_TARGET_SHA	`dbe3d1c`
PR_LABELS	bugfix;AT: AUTOMERGE;kokkos
PULLREQUESTNUM	256
TEST_REPO_ALIAS	EKAT

Build Information

Test Name: EKAT_PullRequest_Autotester_Weaver

Build Num: 447
Status: STARTED

Jenkins Parameters

Parameter Name	Value
EKAT_SOURCE_BRANCH	bartgol/fix-view-reduction
EKAT_SOURCE_REPO	https://github.com/E3SM-Project/EKAT
EKAT_SOURCE_SHA	`4c8d649`
EKAT_TARGET_BRANCH	master
EKAT_TARGET_REPO	https://github.com/E3SM-Project/EKAT
EKAT_TARGET_SHA	`dbe3d1c`
PR_LABELS	bugfix;AT: AUTOMERGE;kokkos
PULLREQUESTNUM	256
TEST_REPO_ALIAS	EKAT

Build Information

Test Name: EKAT_PullRequest_Autotester_Blake

Build Num: 464
Status: STARTED

Jenkins Parameters

Parameter Name	Value
EKAT_SOURCE_BRANCH	bartgol/fix-view-reduction
EKAT_SOURCE_REPO	https://github.com/E3SM-Project/EKAT
EKAT_SOURCE_SHA	`4c8d649`
EKAT_TARGET_BRANCH	master
EKAT_TARGET_REPO	https://github.com/E3SM-Project/EKAT
EKAT_TARGET_SHA	`dbe3d1c`
PR_LABELS	bugfix;AT: AUTOMERGE;kokkos
PULLREQUESTNUM	256
TEST_REPO_ALIAS	EKAT

Using Repos:

Repo: EKAT (E3SM-Project/EKAT)

Pull Request Author: bartgol

E3SM-Autotester · 2022-10-13T00:14:12Z

Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED

Pull Request Auto Testing has PASSED (click to expand)

Build Information

Test Name: EKAT_PullRequest_Autotester_Mappy

Build Num: 350
Status: PASSED

Jenkins Parameters

Parameter Name	Value
EKAT_SOURCE_BRANCH	bartgol/fix-view-reduction
EKAT_SOURCE_REPO	https://github.com/E3SM-Project/EKAT
EKAT_SOURCE_SHA	`4c8d649`
EKAT_TARGET_BRANCH	master
EKAT_TARGET_REPO	https://github.com/E3SM-Project/EKAT
EKAT_TARGET_SHA	`dbe3d1c`
PR_LABELS	bugfix;AT: AUTOMERGE;kokkos
PULLREQUESTNUM	256
TEST_REPO_ALIAS	EKAT

Build Information

Test Name: EKAT_PullRequest_Autotester_Weaver

Build Num: 447
Status: PASSED

Jenkins Parameters

Parameter Name	Value
EKAT_SOURCE_BRANCH	bartgol/fix-view-reduction
EKAT_SOURCE_REPO	https://github.com/E3SM-Project/EKAT
EKAT_SOURCE_SHA	`4c8d649`
EKAT_TARGET_BRANCH	master
EKAT_TARGET_REPO	https://github.com/E3SM-Project/EKAT
EKAT_TARGET_SHA	`dbe3d1c`
PR_LABELS	bugfix;AT: AUTOMERGE;kokkos
PULLREQUESTNUM	256
TEST_REPO_ALIAS	EKAT

Build Information

Test Name: EKAT_PullRequest_Autotester_Blake

Build Num: 464
Status: PASSED

Jenkins Parameters

Parameter Name	Value
EKAT_SOURCE_BRANCH	bartgol/fix-view-reduction
EKAT_SOURCE_REPO	https://github.com/E3SM-Project/EKAT
EKAT_SOURCE_SHA	`4c8d649`
EKAT_TARGET_BRANCH	master
EKAT_TARGET_REPO	https://github.com/E3SM-Project/EKAT
EKAT_TARGET_SHA	`dbe3d1c`
PR_LABELS	bugfix;AT: AUTOMERGE;kokkos
PULLREQUESTNUM	256
TEST_REPO_ALIAS	EKAT

E3SM-Autotester · 2022-10-13T00:14:30Z

Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
THE LAST COMMIT TO THIS PULL REQUEST HAS BEEN REVIEWED, BUT NOT ACCEPTED OR REQUIRES CHANGES

E3SM-Autotester · 2022-10-13T00:14:36Z

All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur...

jgfouca · 2022-10-13T16:31:47Z

@bartgol , note that I also needed to use local variables when using ExeSpaceUtils::parallel_reduce

tcclevenger

Looks good! Should be able to add back to SCREAM now.

E3SM-Autotester · 2022-10-13T17:09:32Z

Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ tcclevenger ]!

E3SM-Autotester · 2022-10-13T17:09:39Z

Status Flag 'Pull Request AutoTester' - Pull Request will be Automerged

E3SM-Autotester · 2022-10-13T17:09:42Z

Merge on Pull Request# 256: IS A SUCCESS - Pull Request successfully merged

bartgol · 2022-10-13T17:16:57Z

@bartgol , note that I also needed to use local variables when using ExeSpaceUtils::parallel_reduce

Ah, it might be that impl::parallel_reduce needs a barrier, to avoid the race condition Andrew pointed out in the shoc small kernel PR in scream:

auto local_var = resullt
// perform reduction into local_var
result = local_var // thread A might execute this line *before* thread B executes the first line

I'll add a fix for that too.

bartgol · 2022-10-13T17:17:53Z

Shoot, the AT beat me. I'll do a super quick separate PR

ambrad · 2022-10-13T17:24:24Z

I didn't get a chance to review before this was merged, but I agree with Jim that this PR is not complete. parallel_reduce needs to be reworked, too.

bartgol · 2022-10-13T17:24:44Z

Yeah, I'm working on it.

jgfouca · 2022-10-13T17:37:38Z

@bartgol , another thought, do you think it would be worth adding a test that would have exposed the prior deficiency?

bartgol · 2022-10-13T17:43:21Z

@bartgol , another thought, do you think it would be worth adding a test that would have exposed the prior deficiency?

That's something I was thinking about, yes. I will add a manifactured test, and verify it would have failed before.

ambrad · 2022-10-13T17:50:56Z

@bartgol @jgfouca another thing I wanted to discuss was my opinion that there should be a version of view_reduction that does not take result as an input, thus negating this issue of automatic variable vs team-global. One could argue that the design flaw was permitting result to contain a valid value on input to be accumulated. That flaw is now leading to team_barrier calls that are strictly needed only when result is a team-global variable.

oksanaguba · 2022-10-13T18:10:36Z

@bartgol what is this PR for? Or, what i really want to know, will there be EKAT changes to scream master soon? Asking because i then would need to pull them into my branch, do not want to miss something. thanks!

bartgol · 2022-10-13T18:52:53Z

This PR requires no change in scream. The follow up one will require a small change in shoc_energy_integrals_impl.hpp. I'll let you know when that happens.

oksanaguba · 2022-10-13T20:10:42Z

Not sure I was clear -- please, let me know when you change head commit of EKAT submodule in scream master. thanks!

bartgol added 3 commits October 12, 2022 15:20

Fix CPP precision settings in kokkos stuff unit tests

9deb8f8

Fix view_reduction in case the result var comes from shared mem

38176bd

Use pack size to 2^n while testing view_reduction

4c8d649

bartgol commented Oct 12, 2022

View reviewed changes

bartgol self-assigned this Oct 12, 2022

bartgol requested review from tcclevenger and ambrad October 12, 2022 23:39

bartgol added bugfix AT: AUTOMERGE Inform the autotester (AT) that it can merge this PR if reviewers approved, and tests pass kokkos labels Oct 12, 2022

tcclevenger approved these changes Oct 13, 2022

View reviewed changes

E3SM-Autotester merged commit 1052776 into master Oct 13, 2022

E3SM-Autotester deleted the bartgol/fix-view-reduction branch October 13, 2022 17:09

E3SM-Autotester removed the AT: AUTOMERGE Inform the autotester (AT) that it can merge this PR if reviewers approved, and tests pass label Oct 13, 2022

Fix view_reduction to work with output variable coming from either private or shared memory #256

Fix view_reduction to work with output variable coming from either private or shared memory #256

Conversation

bartgol commented Oct 12, 2022 • edited Loading

Motivation

Related Issues

E3SM Stakeholder Feedback

bartgol Oct 12, 2022

Choose a reason for hiding this comment

E3SM-Autotester commented Oct 13, 2022

E3SM-Autotester commented Oct 13, 2022

Build Information

Test Name: EKAT_PullRequest_Autotester_Mappy

Jenkins Parameters

Build Information

Test Name: EKAT_PullRequest_Autotester_Weaver

Jenkins Parameters

Build Information

Test Name: EKAT_PullRequest_Autotester_Blake

Jenkins Parameters

Using Repos:

E3SM-Autotester commented Oct 13, 2022

Build Information

Test Name: EKAT_PullRequest_Autotester_Mappy

Jenkins Parameters

Build Information

Test Name: EKAT_PullRequest_Autotester_Weaver

Jenkins Parameters

Build Information

Test Name: EKAT_PullRequest_Autotester_Blake

Jenkins Parameters

E3SM-Autotester commented Oct 13, 2022

E3SM-Autotester commented Oct 13, 2022

jgfouca commented Oct 13, 2022

tcclevenger left a comment

Choose a reason for hiding this comment

E3SM-Autotester commented Oct 13, 2022

E3SM-Autotester commented Oct 13, 2022

E3SM-Autotester commented Oct 13, 2022

bartgol commented Oct 13, 2022 • edited Loading

bartgol commented Oct 13, 2022

ambrad commented Oct 13, 2022

bartgol commented Oct 13, 2022

jgfouca commented Oct 13, 2022

bartgol commented Oct 13, 2022

ambrad commented Oct 13, 2022 • edited Loading

oksanaguba commented Oct 13, 2022 • edited Loading

bartgol commented Oct 13, 2022

oksanaguba commented Oct 13, 2022

bartgol commented Oct 12, 2022 •

edited

Loading

bartgol commented Oct 13, 2022 •

edited

Loading

ambrad commented Oct 13, 2022 •

edited

Loading

oksanaguba commented Oct 13, 2022 •

edited

Loading