This is an archive of the discontinued LLVM Phabricator instance.

[OMPIRBuilder] Support ordered clause specified without parameter
ClosedPublic

Authored by peixin on Dec 2 2021, 3:19 AM.

Details

Summary

This patch supports ordered clause specified without parameter in
worksharing-loop directive in the OpenMPIRBuilder and lowering MLIR to
LLVM IR.

Diff Detail

Unit TestsFailed

Event Timeline

peixin created this revision.Dec 2 2021, 3:19 AM
peixin requested review of this revision.Dec 2 2021, 3:19 AM
Meinersbur added inline comments.Dec 2 2021, 8:39 AM
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
125–130

Did you consider making ordered a mask flag like Monotonic/Nonmonotonic?

peixin added inline comments.Dec 5 2021, 11:39 PM
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
125–130

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".

Meinersbur added inline comments.Dec 7 2021, 7:35 AM
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
125–130

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
1892

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
770

The code would be easier to understand if using a boolean here, e.g. IsOrdered.

845–846

Not sure why you inverted the condition statement?

peixin updated this revision to Diff 393342.Dec 9 2021, 5:11 PM
peixin added a comment.Dec 9 2021, 5:45 PM

Thanks @Meinersbur for the review. Addressed the comments.

llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
125–130

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
1892

Added one new test.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
770

I planed to use "orderedVal" for "ordered(n) (n>0)" clause. That's why I define it as integer. Added TODO.

845–846

No special reason. I followed the logical in clang side to write this code. Reversed back to previous condition statement.

peixin updated this revision to Diff 396006.Dec 23 2021, 4:16 AM

Rebase on main.

Meinersbur added inline comments.Jan 28 2022, 1:29 PM
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
125–130

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

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:
if (64 & schedule) {

schedule = ((int)schedule & ~64) + 32;

}
That is, it resets the kmp_ord flag but sets back that kmp_sch "flag". There is no schedule kind below 32 and there is also no schedule kind above 96 (both "flags" set), so the kmp_sch "flag" would normally not be needed.

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
765
768

Is the ordered incompatible with static schedules?

Meinersbur added inline comments.Jan 28 2022, 2:26 PM
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2025–2028

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.

peixin abandoned this revision.Feb 22 2022, 10:33 PM

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.

peixin reclaimed this revision.Mar 28 2022, 2:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2022, 2:30 AM
peixin updated this revision to Diff 418527.Mar 28 2022, 2:32 AM

Reopen and rebase.

@Meinersbur This is ready for review now.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2025–2028

Fixed.

Meinersbur added inline comments.Mar 29 2022, 3:42 PM
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2018

Prefer consistent style

2021

[style] No Almost-Always-Auto

2022

[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
221–223
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
762–768

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)

831
837–838

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?

peixin updated this revision to Diff 419397.Mar 31 2022, 4:38 AM
peixin marked an inline comment as done.Mar 31 2022, 4:49 AM

Addressed and replied all @Meinersbur 's questions and concerns.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2021

There are a lot of code with this code style issue in OpenMPIRBuilderTest.cpp.

2022

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
762–768

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.

837–838

There is also one your question I forgot to answer.

Is the ordered incompatible with static schedules?

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.

Meinersbur added inline comments.Mar 31 2022, 2:46 PM
llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2022

Would be nice to do so.

For this patch, I am only looking for the coding style of new code.

2055–2057

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
837–838

Please mention this in a comment

peixin updated this revision to Diff 419596.Mar 31 2022, 6:16 PM
peixin marked an inline comment as done.Mar 31 2022, 6:19 PM

Addressed comments.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
2022

OK. Will do it after this patch.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
837–838

Added in comments.

Meinersbur accepted this revision.Mar 31 2022, 7:07 PM

LGTM.

I might have a try on a patch to clean up the switch and OMPScheduleType enum.

This revision is now accepted and ready to land.Mar 31 2022, 7:07 PM
This revision was landed with ongoing or failed builds.Apr 1 2022, 1:20 AM
This revision was automatically updated to reflect the committed changes.
peixin added a comment.Apr 1 2022, 1:21 AM

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

peixin added a comment.Apr 4 2022, 3:49 AM

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.