Page MenuHomePhabricator

[OpenMP]Add support for workshare loop modifier in lowering
ClosedPublic

Authored by Leporacanthicus on May 6 2021, 10:24 AM.

Details

Summary

When lowering the dynamic, guided, auto and runtime types of scheduling,
there is an optional monotonic or non-monotonic modifier. This patch
adds support in the OMP IR Builder to pass this down to the runtime
functions.

Also implements tests for the variants.

Diff Detail

Event Timeline

Leporacanthicus requested review of this revision.May 6 2021, 10:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2021, 10:24 AM

Patch doesn't apply because it is relying on my previous patch. Doh!

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1435–1441

@Meinersbur & @jdoerfert

Does this section make sense: use non-monotonic as default for guided and dynamic, and monotonic for Auto and Runtime? From what I can tell, this is what Clang does, and the Fortran compiler complains if I try to use non-monotonic with Auto and Runtime.

Patch doesn't apply because it is relying on my previous patch. Doh!

Add the other patch as parent in Phabricator

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1435–1441

Try to mimic what the current OpenMP implementation does:

// OpenMP 5.0, 2.9.2 Worksharing-Loop Construct, Desription.
// If the static schedule kind is specified or if the ordered clause is
// specified, and if the nonmonotonic modifier is not specified, the effect is
// as if the monotonic modifier is specified. Otherwise, unless the monotonic
// modifier is specified, the effect is as if the nonmonotonic modifier is
// specified.
if (CGM.getLangOpts().OpenMP >= 50 && Modifier == 0) {
  if (!(Schedule == OMP_sch_static_chunked || Schedule == OMP_sch_static ||
        Schedule == OMP_sch_static_balanced_chunked ||
        Schedule == OMP_ord_static_chunked || Schedule == OMP_ord_static ||
        Schedule == OMP_dist_sch_static_chunked ||
        Schedule == OMP_dist_sch_static))
    Modifier = OMP_sch_modifier_nonmonotonic;
}
return Schedule | Modifier;

i.e. add implicit nonmonotonic only for static schedules and if user did not explicitly specify a modifier. This is createDynamicWorkshareLoop so I guess no implicit modifier should be added at all.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
1800–1807

Copy&Pasting the code into the tests doesn't seem useful. I'd prefer checking some values and their expected result.

Rebase and fixed comment style.

Remove the code that add in modifiers to the scheduling type,
as per review comments.

Leporacanthicus marked an inline comment as done.May 14 2021, 5:45 AM
Leporacanthicus added inline comments.
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1435–1441

Ok, I've removed all the modifier-add-in.

Thanks for your advice!

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
1800–1807

Removed this in the updated patch.

Leporacanthicus marked an inline comment as done.Tue, May 18, 6:40 AM

@Meinersbur Is there anything else I need to do here?

Meinersbur accepted this revision.Tue, May 25, 7:15 AM

LGTM, thank you.

llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
121

Line comment does not need to end with */

This revision is now accepted and ready to land.Tue, May 25, 7:15 AM

Removed wrong comment endings.

Leporacanthicus marked an inline comment as done.Wed, May 26, 3:07 AM
Leporacanthicus added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
121

Thanks. I thought I'd fixed that earlier, but was probably on a different branch.

Meinersbur accepted this revision.Wed, May 26, 7:35 AM