Page MenuHomePhabricator

[OpenMP] Refactor OMPScheduleType enum.
ClosedPublic

Authored by Meinersbur on Apr 8 2022, 9:33 AM.

Details

Summary

The OMPScheduleType enum stores the constants from libomp's internal sched_type in kmp.h and are used by several kmp API functions. The enum values have an internal structure, namely each scheduling algorithm (e.g.) exists in four variants: unordered, orderend, normerge unordered, and nomerge ordered.

This patch (basically a followup to D114940) splits the "ordered" and "nomerge" bits into separate flags, as was already done for the "monotonic" and "nonmonotonic", so we can apply bit flags operations on them. It also now contains all possible combinations according to kmp's sched_type. Deriving of the OMPScheduleType enum from clause parameters has been moved form MLIR's OpenMPToLLVMIRTranslation.cpp to OpenMPIRBuilder to make available for clang as well. Since the primary purpose of the flag is the binary interface to libomp, it has been made more private to LLVMFrontend. The primary interface for generating worksharing-loop using OpenMPIRBuilder code becomes applyWorkshareLoop which derives the OMPScheduleType automatically and calls the appropriate emitter function.

While this is mostly a NFC refactor, it still applies the following functional changes:

  • The logic from OpenMPToLLVMIRTranslation to derive the OMPScheduleType also applies to clang. Most notably, it now applies the nonmonotonic flag for non-static schedules by default.
  • In OpenMPToLLVMIRTranslation, the nonmonotonic default flag was previously not applied if the simd modifier was used. I assume this was a bug, since the effect was due to loop.schedule_modifier() returning mlir::omp::ScheduleModifier::none instead of llvm::Optional::None.
  • In OpenMPToLLVMIRTranslation, the nonmonotonic default flag was set even if ordered was specified, in breach to what the comment before citing the OpenMP specification says. I assume this was an oversight.

The ordered flag with parameter was not considered in this patch. Changes will need to be made (e.g. adding/modifying function parameters) when support for it is added. The lengthy names of the enum values can be discussed, for the moment this is avoiding reusing previously existing enum value names such as StaticChunked to avoid confusion.

Diff Detail

Event Timeline

Meinersbur created this revision.Apr 8 2022, 9:33 AM
Herald added a project: Restricted Project. · View Herald Transcript
Meinersbur requested review of this revision.Apr 8 2022, 9:33 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptApr 8 2022, 9:33 AM

A few comments. Mostly nits.

clang/lib/CodeGen/CGStmtOpenMP.cpp
3767
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
87
BaseRuntime = 5,
BaseAuto = 6,
135

Why not using the following to be consistent with the name in kmp.h?
StaticBalancedChunked
GuidedSimd
RuntimeSimd

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2114

Nit

2254
2255
2256
mlir/test/Target/LLVMIR/openmp-llvm.mlir
809

Is this one tab?

Meinersbur marked 7 inline comments as done.Apr 14 2022, 7:10 AM
Meinersbur added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
135

As mentioned in the summary. to avoid confusion by not using the original name. StaticBalancedChunked could mean either the algorithm to use (now BaseStaticBalancedChunked, as in omp_sched_t/enum kmp_sched), or that algorithm with the unordered flag set (now UnorderedStaticBalancedChunked ). I would the former because that's how the enum is structured.

The name in kmp.h for it is actually kmp_sch_static_balanced_chunked. sch for "unordered"?

  • address review
peixin accepted this revision.Apr 14 2022, 6:02 PM
peixin added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
135

OK. Agree.

The name in kmp.h for it is actually kmp_sch_static_balanced_chunked. sch for "unordered"?

Yes, I think so. With simd modifier, it cannot be ordered in semantics.

This revision is now accepted and ready to land.Apr 14 2022, 6:02 PM
This revision was landed with ongoing or failed builds.Apr 18 2022, 12:04 PM
This revision was automatically updated to reflect the committed changes.