This patch supports ordered clause specified without parameter in
worksharing-loop directive in the OpenMPIRBuilder and lowering MLIR to
LLVM IR.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h | ||
---|---|---|
90–95 | Did you consider making ordered a mask flag like Monotonic/Nonmonotonic? |
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h | ||
---|---|---|
90–95 | 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". |
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h | ||
---|---|---|
90–95 | 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. | |
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp | ||
2098 | 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. | |
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | ||
813 | The code would be easier to understand if using a boolean here, e.g. IsOrdered. | |
892–893 | Not sure why you inverted the condition statement? |
Thanks @Meinersbur for the review. Addressed the comments.
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h | ||
---|---|---|
90–95 | 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? | |
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp | ||
2098 | Added one new test. | |
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | ||
813 | I planed to use "orderedVal" for "ordered(n) (n>0)" clause. That's why I define it as integer. Added TODO. | |
892–893 | No special reason. I followed the logical in clang side to write this code. Reversed back to previous condition statement. |
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h | ||
---|---|---|
90–95 |
schedule = (enum sched_type)(((int)schedule) - (kmp_ord_lower - kmp_sch_lower)); Can be seen as a hidden bit operation. With same equalities knowing the 64 bit is set: schedule = ((int)schedule & ~64) + 32; } kmp_nm and kmp_nm_ord conforms this pattern as well. What I was suggesting is something like enum class OMPScheduleType { StaticChunked = 1, Static = 2, // static unspecialized DynamicChunked = 3, GuidedChunked = 4, // guided unspecialized Runtime = 5, Auto = 6, // auto StaticBalancedChunked = 13, // static with chunk adjustment (e.g., simd) GuidedSimd = 14, // guided with chunk adjustment RuntimeSimd = 15, // runtime with chunk adjustment DistributeChunked = 27, Distribute = 28, ModifierUnordered = (1 << 5), ModifierOrdered = (1 << 6), ModifierNomerge = (1 << 7), ModifierMonotonic = (1 << 29), // Set if the monotonic schedule modifier was present ModifierNonmonotonic = (1 << 30), // Set if the nonmonotonic schedule modifier was present ModifierMask = ModifierMonotonic | ModifierNonmonotonic | ModifierUnordered | ModifierOrdered | ModifierNm, LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue */ ModifierMask) However, as you mentioned it deviates from the exiting enum in kmp.h which might be confusing. So maybe lets ignore this possibility for now, although it would simplify the giant switch case a lot. | |
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | ||
802–810 | Is the ordered incompatible with static schedules? | |
805 |
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp | ||
---|---|---|
2231–2234 | I think assuming a specific order of allocas and other instructions is not that useful. They break easily with any change and are hard to repair since the instruction order has no meaning. The code here effectively only tests "there are 4 alloca instructions at the beginning of the function" I would prefer we only have few, but significant tests such as "there is a call to __kmpc_dispatch_init_4u somewhere". Even searching for an instruction with a specific name is more meaningful than relying on the order. |
In last OpenMP Flang technical call, got the information from OpenMP community by @Meinersbur that implementation of ordered directive and clause is under discussion. Currently in LLVM openmp library and clang frontend, the doacross loop is independent from the worksharing loop. The OpenMP community is discussing about if fixing the canonical loop instead of forming one new doacross loop considering the performance issue and edge cases such as overflow. We plan to delay the progress of lowering the ordered directive and clause. So close this PR for now and may reopen this in future.
@Meinersbur This is ready for review now.
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp | ||
---|---|---|
2231–2234 | Fixed. |
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp | ||
---|---|---|
2224 | Prefer consistent style | |
2227 | [style] No Almost-Always-Auto | |
2228 | [style] LLVM coding style starts local variables with a capital letter. But preferable, just use EI.member instead of cur->member. | |
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
283–285 | Just confirming this is correct: https://www.openmp.org/spec-html/5.1/openmpsu48.html#x73-730002.11.4 | |
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | ||
802–810 | Since ordered has quite different meaning than ordered(k). I'd prefer to split them into two different variables. I don't insist on it, but please add a comment on the meanings of orderedVal and the ordered clause, maybe by citing from the the OpenMP spec (for both meanings, not just the "OpenMP 5.1, 2.11.4 " below) | |
876 | ||
882–883 | This doesn't seem right. It says "if the ordered clause is specified", but this does not consider the orderedVal. What sets the Monotonic modifier? |
Addressed and replied all @Meinersbur 's questions and concerns.
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp | ||
---|---|---|
2227 | There are a lot of code with this code style issue in OpenMPIRBuilderTest.cpp. | |
2228 | There are a lot of code with this code style issue in OpenMPIRBuilderTest.cpp. Should I create one NFC patch to fix it in case it guide followers using the wrong style? | |
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | ||
802–810 | I add some comments. Please check if it is ok to you? You can also fix this to a better one when you support doacross loops. | |
882–883 | There is also one your question I forgot to answer.
For these two questions, I think this is the history issue of clang and openmp runtime library. The Monotonic is used by default in openmp runtime library if it is not set here. The ordered is compatible with static schedules. But this code is following what clang does. |
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp | ||
---|---|---|
2228 | Would be nice to do so. For this patch, I am only looking for the coding style of new code. | |
2261–2263 | Finalization (and return) of the function should occur before the EXPECT macros. After all, we want to verify the correctness of the function "when it is ready to use" | |
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | ||
882–883 | Please mention this in a comment |
I might have a try on a patch to clean up the switch and OMPScheduleType enum.
It would be great.
Hi @peixin,
This change has caused a build error when building with clang:
/usr/bin/clang++ -DGTEST_HAS_RTTI=0 -DMLIR_CUDA_CONVERSIONS_ENABLED=0 -DMLIR_ROCM_CONVERSIONS_ENABLED=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/mnt/vss/_work/1/b/llvm/Release/tools/mlir/lib/Target/LLVMIR/Dialect/OpenMP -I/mnt/vss/_work/1/llvm-project/mlir/lib/Target/LLVMIR/Dialect/OpenMP -I/mnt/vss/_work/1/b/llvm/Release/include -I/mnt/vss/_work/1/llvm-project/llvm/include -I/mnt/vss/_work/1/llvm-project/mlir/include -I/mnt/vss/_work/1/b/llvm/Release/tools/mlir/include -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -Werror=mismatched-tags -Werror=global-constructors -O3 -DNDEBUG -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -MT tools/mlir/lib/Target/LLVMIR/Dialect/OpenMP/CMakeFiles/obj.MLIROpenMPToLLVMIRTranslation.dir/OpenMPToLLVMIRTranslation.cpp.o -MF tools/mlir/lib/Target/LLVMIR/Dialect/OpenMP/CMakeFiles/obj.MLIROpenMPToLLVMIRTranslation.dir/OpenMPToLLVMIRTranslation.cpp.o.d -o tools/mlir/lib/Target/LLVMIR/Dialect/OpenMP/CMakeFiles/obj.MLIROpenMPToLLVMIRTranslation.dir/OpenMPToLLVMIRTranslation.cpp.o -c /mnt/vss/_work/1/llvm-project/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp /mnt/vss/_work/1/llvm-project/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:854:5: error: default label in switch which covers all enumeration values [-Werror,-Wcovered-switch-default] default: ^ 1 error generated.
Thanks,
Henry
Thanks, Henry. There is OMPC_SCHEDULE_unknown in clang frontend, but there is no such definition in MLIR. So I missed it. Fix this in https://reviews.llvm.org/D123018.
Did you consider making ordered a mask flag like Monotonic/Nonmonotonic?