Page MenuHomePhabricator

[OpenMP] Avoid reading uninitialized parallel level values
ClosedPublic

Authored by jdoerfert on Apr 22 2021, 6:43 PM.

Details

Summary

In a last minute change request for a2dbfb6b72db we introduced a
read of the uninitialized parallel level value in SPMD-mode.
We go back to initializing the array early and checking for an
adjusted level.

Found by the miniqmc unit tests:

https://cdash.qmcpack.org/CDash/viewTest.php?onlyfailed&buildid=203434

Diff Detail

Event Timeline

jdoerfert created this revision.Apr 22 2021, 6:43 PM
jdoerfert requested review of this revision.Apr 22 2021, 6:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2021, 6:43 PM
Herald added a subscriber: sstefan1. · View Herald Transcript
JonChesterfield accepted this revision.Apr 22 2021, 6:55 PM
JonChesterfield added a subscriber: JonChesterfield.

This fix looks sound. D95976 itself is quite complicated, don't have a handle on what that is doing yet.

This revision is now accepted and ready to land.Apr 22 2021, 6:55 PM

Does this work correctly with a nested parallel region in SPMD mode? I think the zeroing-out of the parallel-level is needed for this case.

Does this work correctly with a nested parallel region in SPMD mode? I think the zeroing-out of the parallel-level is needed for this case.

The parallel level in SPMD mode can never be zero. It is 1 in the beginning and then can only be higher as we enter serialized parallel regions (Inc/Dec level in *_serialized_parallel).

Does this work correctly with a nested parallel region in SPMD mode? I think the zeroing-out of the parallel-level is needed for this case.

The parallel level in SPMD mode can never be zero. It is 1 in the beginning and then can only be higher as we enter serialized parallel regions (Inc/Dec level in *_serialized_parallel).

I don't think it's going to enter the serialized parallel execution because the parallelLevel is not incremented in the parallel call. Perhaps we need to init parallelLevel in spmd_init to 0, if that doesn't cause any problems in other parts of the runtime, and leave the handling of the parallelLevel inside the parallel call. We should also add a test for nested parallel regions in SPMD mode too.