[OpenMP]Add support for workshare loop modifier in lowering

[Leporacanthicus]

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.

[Leporacanthicus]

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.

[Meinersbur]

In D102008#2744138, @Leporacanthicus wrote:

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.

[Leporacanthicus]

Rebase and fixed comment style.

[Leporacanthicus]

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

[Leporacanthicus]

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]

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

[Meinersbur]

LGTM, thank you.

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

121

Line comment does not need to end with */

[Leporacanthicus]

Removed wrong comment endings.

[Leporacanthicus]

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

121

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

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

 ///       To complete this, more enum values will need to be moved here.
 enum class OMPScheduleType {
   Static = 34, // static unspecialized
   DynamicChunked = 35,
   GuidedChunked = 36, // guided unspecialized
   Runtime = 37,
   Auto = 38, // auto
 
+  ModifierMonotonic =
+      (1 << 29), // Set if the monotonic schedule modifier was present
   ModifierNonmonotonic =
-      (1 << 30), /**< Set if the nonmonotonic schedule modifier was present */
-
-  LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue */ ModifierNonmonotonic)
+      (1 << 30), // Set if the nonmonotonic schedule modifier was present
+  ModifierMask = ModifierMonotonic | ModifierNonmonotonic,
+  LLVM_MARK_AS_BITMASK_ENUM(/* LargestValue */ ModifierMask)
 };
 
 } // end namespace omp
 
 } // end namespace llvm
 
 #endif // LLVM_FRONTEND_OPENMP_OMPCONSTANTS_H

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::createDynamicWorkshareLoop(
   // The CLI will be "broken" in the code below, as the loop is no longer
   // a valid canonical loop.
 
   if (!Chunk)
     Chunk = One;
 
   Value *ThreadNum = getOrCreateThreadID(SrcLoc);
 
-  OMPScheduleType DynamicSchedType =
-      SchedType | OMPScheduleType::ModifierNonmonotonic;
   Constant *SchedulingType =
-      ConstantInt::get(I32Type, static_cast<int>(DynamicSchedType));
+      ConstantInt::get(I32Type, static_cast<int>(SchedType));
 
   // Call the "init" function.
   Builder.CreateCall(DynamicInit,
                      {SrcLoc, ThreadNum, SchedulingType, /* LowerBound */ One,
                       UpperBound, /* step */ One, Chunk});
 
   // An outer loop around the existing one.
   BasicBlock *OuterCond = BasicBlock::Create(
       PreHeader->getContext(), Twine(PreHeader->getName()) + ".outer.cond",
       PreHeader->getParent());
   // This needs to be 32-bit always, so can't use the IVTy Zero above.
   Builder.SetInsertPoint(OuterCond, OuterCond->getFirstInsertionPt());
   Value *Res =
       Builder.CreateCall(DynamicNext, {SrcLoc, ThreadNum, PLastIter,

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

TEST_P(OpenMPIRBuilderTestWithParams, DynamicWorkShareLoop) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
   OMPBuilder.initialize();
   IRBuilder<> Builder(BB);
   OpenMPIRBuilder::LocationDescription Loc({Builder.saveIP(), DL});
 
   omp::OMPScheduleType SchedType = GetParam();
   uint32_t ChunkSize = 1;
-  switch (SchedType) {
+  switch (SchedType & ~omp::OMPScheduleType::ModifierMask) {
   case omp::OMPScheduleType::DynamicChunked:
   case omp::OMPScheduleType::GuidedChunked:
     ChunkSize = 7;
     break;
   case omp::OMPScheduleType::Auto:
   case omp::OMPScheduleType::Runtime:
     ChunkSize = 1;
     break;
EXPECT_EQ(ThreadIdCall->getCalledFunction()->getName(),
             "__kmpc_global_thread_num");
 
   CallInst *InitCall = dyn_cast<CallInst>(&*PreheaderIter);
 
   ASSERT_NE(InitCall, nullptr);
   EXPECT_EQ(InitCall->getCalledFunction()->getName(),
             "__kmpc_dispatch_init_4u");
   EXPECT_EQ(InitCall->getNumArgOperands(), 7U);
-  EXPECT_EQ(InitCall->getArgOperand(6),
-            ConstantInt::get(Type::getInt32Ty(Ctx), ChunkSize));
+  EXPECT_EQ(InitCall->getArgOperand(6), ConstantInt::get(LCTy, ChunkSize));
+  ConstantInt *SchedVal = cast<ConstantInt>(InitCall->getArgOperand(2));
+  EXPECT_EQ(SchedVal->getValue(), static_cast<uint64_t>(SchedType));
 
   ConstantInt *OrigLowerBound =
       dyn_cast<ConstantInt>(LowerBoundStore->getValueOperand());
   ConstantInt *OrigUpperBound =
       dyn_cast<ConstantInt>(UpperBoundStore->getValueOperand());
   ConstantInt *OrigStride =
       dyn_cast<ConstantInt>(StrideStore->getValueOperand());
   ASSERT_NE(OrigLowerBound, nullptr);
   ASSERT_NE(OrigUpperBound, nullptr);
   ASSERT_NE(OrigStride, nullptr);
   EXPECT_EQ(OrigLowerBound->getValue(), 1);
   EXPECT_EQ(OrigUpperBound->getValue(), 21);
   EXPECT_EQ(OrigStride->getValue(), 1);
 
   // The original loop iterator should only be used in the condition, in the
   // increment and in the statement that adds the lower bound to it.
   EXPECT_EQ(std::distance(IV->use_begin(), IV->use_end()), 3);
 
   // The exit block should contain the barrier call, plus the call to obtain
   // the thread ID.
   size_t NumCallsInExitBlock =
       count_if(*ExitBlock, [](Instruction &I) { return isa<CallInst>(I); });
   EXPECT_EQ(NumCallsInExitBlock, 2u);
 
   // Add a termination to our block and check that it is internally consistent.
   Builder.restoreIP(EndIP);
   Builder.CreateRetVoid();
   OMPBuilder.finalize();
   EXPECT_FALSE(verifyModule(*M, &errs()));
 }
 
-INSTANTIATE_TEST_SUITE_P(OpenMPWSLoopSchedulingTypes,
-                        OpenMPIRBuilderTestWithParams,
-                        ::testing::Values(omp::OMPScheduleType::DynamicChunked,
-                                          omp::OMPScheduleType::GuidedChunked,
-                                          omp::OMPScheduleType::Auto,
-                                          omp::OMPScheduleType::Runtime));
+INSTANTIATE_TEST_CASE_P(
+    OpenMPWSLoopSchedulingTypes, OpenMPIRBuilderTestWithParams,
+                      omp::OMPScheduleType::Auto, omp::OMPScheduleType::Runtime,
+                      omp::OMPScheduleType::DynamicChunked |
+                          omp::OMPScheduleType::ModifierMonotonic,
+                      omp::OMPScheduleType::DynamicChunked |
+                          omp::OMPScheduleType::ModifierNonmonotonic,
+                      omp::OMPScheduleType::GuidedChunked |
+                          omp::OMPScheduleType::ModifierMonotonic,
+                      omp::OMPScheduleType::GuidedChunked |
+                          omp::OMPScheduleType::ModifierNonmonotonic,
+                      omp::OMPScheduleType::Auto |
+                          omp::OMPScheduleType::ModifierMonotonic,
+                      omp::OMPScheduleType::Runtime |
+                          omp::OMPScheduleType::ModifierMonotonic));
 
 TEST_F(OpenMPIRBuilderTest, MasterDirective) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;
   OpenMPIRBuilder OMPBuilder(*M);
   OMPBuilder.initialize();
   F->setName("func");
   IRBuilder<> Builder(BB);