[PATCH] D114940: [OMPIRBuilder] Support ordered clause specified without parameter
Peixin Qiao via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 9 17:45:11 PST 2021
peixin added a comment.
Thanks @Meinersbur for the review. Addressed the comments.
================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:125-130
+ OrderedStaticChunked = 65,
+ OrderedStatic = 66, // ordered static unspecialized
+ OrderedDynamicChunked = 67,
+ OrderedGuidedChunked = 68,
+ OrderedRuntime = 69,
+ OrderedAuto = 70, // ordered auto
----------------
Meinersbur wrote:
> peixin wrote:
> > Meinersbur wrote:
> > > Did you consider making `ordered` a mask flag like Monotonic/Nonmonotonic?
> > These values are copied from "kmp.h". I prefer to keep the originl definition rather than using a mask flag. In addition, the ordered values don't have the consistent mask such as kmp_sch_upper and kmp_ord_upper in "kmp.h".
> There are used as flags though: https://github.com/llvm/llvm-project/blob/c94eb0f9ef5597bd1b3b0efda4df53f606a1fe13/openmp/runtime/src/kmp_dispatch.cpp#L185
>
> However, there is an argument to keep the different definitions similar to each other. Eventually I hope those constant can come from a single source.
It's true that "ordered" is used as flag in runtime library, but the schedule is updated by subtracting the offset instead of using one mask: https://github.com/llvm/llvm-project/blob/c94eb0f9ef5597bd1b3b0efda4df53f606a1fe13/openmp/runtime/src/kmp_dispatch.cpp#L187-L188.
I tried to use the offset, and the code would be like the following:
```
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMPConstants.h b/llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
index d2f9bac16e5a..adc5891b91f2 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
+++ b/llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
@@ -113,8 +113,6 @@ inline std::string getAllAssumeClauseOptions() {
enum class OMPScheduleType {
StaticChunked = 33,
Static = 34, // static unspecialized
- DistributeChunked = 91,
- Distribute = 92,
DynamicChunked = 35,
GuidedChunked = 36, // guided unspecialized
Runtime = 37,
@@ -124,6 +122,19 @@ enum class OMPScheduleType {
GuidedSimd = 46, // guided with chunk adjustment
RuntimeSimd = 47, // runtime with chunk adjustment
+ OrderedOffset = 32, // offset for StaticChunked, Static, DynamicChunked,
+ // GuidedChunked, Runtime and Auto
+ OrderedSimdOffset = 22, // offset for GuidedSimd and RuntimeSimd
+ OrderedStaticChunked = 65,
+ OrderedStatic = 66, // ordered static unspecialized
+ OrderedDynamicChunked = 67,
+ OrderedGuidedChunked = 68,
+ OrderedRuntime = 69,
+ OrderedAuto = 70, // ordered auto
+
+ DistributeChunked = 91, // distribute static chunked
+ Distribute = 92, // distribute static unspecialized
+
ModifierMonotonic =
(1 << 29), // Set if the monotonic schedule modifier was present
ModifierNonmonotonic =
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 38f93f7faf92..bdde5edf79b1 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -687,12 +687,27 @@ convertOmpWsLoop(Operation &opInst, llvm::IRBuilderBase &builder,
bool isSimd = loop.simd_modifier();
- if (schedule == omp::ClauseScheduleKind::Static) {
+ // TODO: Emit kmpc_doacross_init call before kmpc_for_static_init call or
+ // kmpc_dispatch_init call with the argument of orderedVal when orderedVal is
+ // greater than 0.
+ std::int64_t orderedVal =
+ loop.ordered_val().hasValue() ? loop.ordered_val().getValue() : -1;
+ if (schedule == omp::ClauseScheduleKind::Static && orderedVal != 0) {
ompBuilder->applyStaticWorkshareLoop(ompLoc.DL, loopInfo, allocaIP,
!loop.nowait(), chunk);
} else {
llvm::omp::OMPScheduleType schedType;
switch (schedule) {
+ case omp::ClauseScheduleKind::Static:
+ if (isSimd) {
+ schedType = llvm::omp::OMPScheduleType::StaticBalancedChunked;
+ } else {
+ if (loop.schedule_chunk_var())
+ schedType = llvm::omp::OMPScheduleType::StaticChunked;
+ else
+ schedType = llvm::omp::OMPScheduleType::Static;
+ }
+ break;
case omp::ClauseScheduleKind::Dynamic:
schedType = llvm::omp::OMPScheduleType::DynamicChunked;
break;
@@ -716,6 +731,11 @@ convertOmpWsLoop(Operation &opInst, llvm::IRBuilderBase &builder,
break;
}
+ if (orderedVal == 0 && !isSimd)
+ schedType = (llvm::omp::OMPScheduleType)(static_cast<int>(schedType) + static_cast<int>(llvm::omp::OMPScheduleType::OrderedOffset));
+ else if (orderedVal == 0 && isSimd)
+ schedType = (llvm::omp::OMPScheduleType)(static_cast<int>(schedType) + static_cast<int>(llvm::omp::OMPScheduleType::OrderedSimdOffset));
+
if (loop.schedule_modifier().hasValue()) {
omp::ScheduleModifier modifier =
*omp::symbolizeScheduleModifier(loop.schedule_modifier().getValue());
@@ -730,9 +750,21 @@ convertOmpWsLoop(Operation &opInst, llvm::IRBuilderBase &builder,
// Nothing to do here.
break;
}
+ } else {
+ // OpenMP 5.1, 2.11.4 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 (!(static_cast<int>(schedType) == static_cast<int>(llvm::omp::OMPScheduleType::Static) + static_cast<int>(llvm::omp::OMPScheduleType::OrderedOffset) ||
+ static_cast<int>(schedType) == static_cast<int>(llvm::omp::OMPScheduleType::StaticChunked) + static_cast<int>(llvm::omp::OMPScheduleType::OrderedOffset) ||
+ static_cast<int>(schedType) == static_cast<int>(llvm::omp::OMPScheduleType::StaticBalancedChunked) + static_cast<int>(llvm::omp::OMPScheduleType::OrderedSimdOffset)))
+ schedType |= llvm::omp::OMPScheduleType::ModifierNonmonotonic;
}
afterIP = ompBuilder->applyDynamicWorkshareLoop(
- ompLoc.DL, loopInfo, allocaIP, schedType, !loop.nowait(), chunk);
+ ompLoc.DL, loopInfo, allocaIP, schedType, !loop.nowait(), chunk,
+ /*ordered*/ orderedVal == 0);
}
// Continue building IR after the loop. Note that the LoopInfo returned by
```
This code looks kind of ugly. What do you think?
================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1892
+ /*NeedsBarrier=*/true, ChunkVal,
+ /*Ordered*/ true);
// The returned value should be the "after" point.
----------------
Meinersbur wrote:
> This modifies an existing test to only check with the ordered flag enabled. You could either make an new test for ordered or parametrize this test for with/without ordered.
Added one new test.
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:690
- if (schedule == omp::ClauseScheduleKind::Static) {
- ompBuilder->applyStaticWorkshareLoop(ompLoc.DL, loopInfo, allocaIP,
- !loop.nowait(), chunk);
- } else {
+ std::int64_t orderedVal =
+ loop.ordered_val().hasValue() ? loop.ordered_val().getValue() : -1;
----------------
Meinersbur wrote:
> The code would be easier to understand if using a boolean here, e.g. `IsOrdered`.
I planed to use "orderedVal" for "ordered(n) (n>0)" clause. That's why I define it as integer. Added TODO.
================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:767-768
+ } else {
+ ompBuilder->applyStaticWorkshareLoop(ompLoc.DL, loopInfo, allocaIP,
+ !loop.nowait(), chunk);
}
----------------
Meinersbur wrote:
> Not sure why you inverted the condition statement?
No special reason. I followed the logical in clang side to write this code. Reversed back to previous condition statement.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114940/new/
https://reviews.llvm.org/D114940
More information about the llvm-commits
mailing list