[OpenMP][Flang]Add support for modifiers monotonic and non-monotonic #791

Leporacanthicus · 2021-05-21T13:29:43Z

This is a package of 4 patches, three of which are already in LLVM (support for modifiers at time of writing in review). The last one of these changes brings the modifiers from the source code to the LLVM translation.

jeanPerier · 2021-05-21T14:26:53Z

Hi, thanks for the patch. I am working on a full rebase of fir-dev on llvm main that I would like to get in probably next Tuesday. It will include the first three commits from LLVM. If it's OK for you to wait until then, it would be easier for me to not have this PR in before the rebase (which mean you will probably have to cherry-pick your last commit into the fresh fir-dev and make a new PR/update this one afterwards). It does not prevent others from reviewing your last commit specific to fir-dev already.

schweitzpgi

Thank you for the PR. Looks good. Just a few minor nitpick comments.

schweitzpgi · 2021-05-25T15:20:03Z

flang/lib/Lower/OpenMP.cpp

+    return mlir::omp::ClauseScheduleModifier::Monotonic;
+  case Fortran::parser::OmpScheduleModifierType::ModType::Nonmonotonic:
+    return mlir::omp::ClauseScheduleModifier::Nonmonotonic;
+  default:


Just an FYI. At one time, flang was using -Wswitch as an error to handle some unreachable cases at compile time.

schweitzpgi · 2021-05-25T15:24:21Z

flang/lib/Lower/OpenMP.cpp

+  if (modifier) {
+    const auto &modType1 =
+        std::get<Fortran::parser::OmpScheduleModifier::Modifier1>(modifier->t);
+    // TODO: Add support for SIMD, which means modType2 gets used.


Should this be a hard TODO() here? (See flang/lower/Todo.h..) In general, we prefer that flang exits rather than emits incorrect code silently.

As far as I understand it,it won't generate incorrect code [functionality-wise] by skipping the SIMD modifier. It may be less efficient than the intent from the programmer... :)

flang/lib/Lower/OpenMP.cpp

flang/lib/Semantics/check-omp-structure.cpp

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

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

jeanPerier · 2021-05-27T16:05:49Z

Hi, thanks for the patch. I am working on a full rebase of fir-dev on llvm main that I would like to get in probably next Tuesday. It will include the first three commits from LLVM. If it's OK for you to wait until then, it would be easier for me to not have this PR in before the rebase (which mean you will probably have to cherry-pick your last commit into the fresh fir-dev and make a new PR/update this one afterwards). It does not prevent others from reviewing your last commit specific to fir-dev already.

Rebase was just done, so you should just be able to cherry-pick your last commit into fir-dev head now without bringing LLVM commits.

Leporacanthicus · 2021-06-02T13:49:36Z

Hi, thanks for the patch. I am working on a full rebase of fir-dev on llvm main that I would like to get in probably next Tuesday. It will include the first three commits from LLVM. If it's OK for you to wait until then, it would be easier for me to not have this PR in before the rebase (which mean you will probably have to cherry-pick your last commit into the fresh fir-dev and make a new PR/update this one afterwards). It does not prevent others from reviewing your last commit specific to fir-dev already.

Rebase was just done, so you should just be able to cherry-pick your last commit into fir-dev head now without bringing LLVM commits.

Thanks, I'm working on an update. Currently not compiling, probably something I broke in the rebase! :(

Leporacanthicus · 2021-06-10T17:32:39Z

Right, finally got this updated to the current version of F18.

I believe I've fixed all previous review comments.

kiranchandramohan

Have a few minor comments and one request for change.

llvm/include/llvm/Frontend/OpenMP/OMP.td

kiranchandramohan · 2021-06-14T21:17:42Z

flang/lib/Semantics/check-omp-structure.cpp

@@ -1404,6 +1405,10 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Schedule &x) {
  }
 }

+void OmpStructureChecker::Enter(const parser::OmpClause::ScheduleModifier &x) {


Can you use CHECK_SIMPLE_CLAUSE instead of writing this function?

Since it's no longer a clause, not really needed.

clang/lib/CodeGen/CGStmtOpenMP.cpp

kiranchandramohan · 2021-06-14T21:20:23Z

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

@@ -299,6 +299,22 @@ convertOmpWsLoop(Operation &opInst, llvm::IRBuilderBase &builder,
      break;
    }

+    if (loop.schedule_modifiers().hasValue()) {


Do we need a TODO here also for multiple modifiers? Also in print and parse?

The TODO was mainly to explain why there was commented out code. SIMD will just add a bit more code here... :)

kiranchandramohan · 2021-06-14T21:28:02Z

mlir/test/Dialect/OpenMP/ops.mlir

  omp.wsloop (%iv) : index = (%lb) to (%ub) step (%step) ordered(2) private(%data_var : memref<i32>)
     firstprivate(%data_var : memref<i32>) lastprivate(%data_var : memref<i32>) linear(%data_var = %linear_var : memref<i32>)
-     schedule(static = %chunk_var) collapse(3) {
+     schedule(static = %chunk_var, None) collapse(3) {
    omp.yield
  }


Add a modifier test here as well.
And also in mlir/test/Target/LLVMIR/openmp-llvm.mlir
While it looks excessive, i think the convention in llvm projects is that things should be independently separately tested.

flang/lib/Lower/OpenMP.cpp

kiranchandramohan · 2021-07-06T20:36:13Z

mlir/test/Target/LLVMIR/openmp-llvm.mlir

+}
+
+llvm.func @test_omp_wsloop_dynamic_monotonic(%lb : i64, %ub : i64, %step : i64) -> () {
+ omp.wsloop (%iv) : i64 = (%lb) to (%ub) step (%step) schedule(dynamic, Monotonic) {


Should we have the same case for the schedule type and the modifier?

Yes, good point.

kiranchandramohan · 2021-07-06T20:37:43Z

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

+	    auto mod = parser.getBuilder().getStringAttr(modifiers[0]);
+	    result.addAttribute("schedule_modifiers", mod);
+            if (modifiers.size() > 1) {
+              mod = parser.getBuilder().getStringAttr(modifiers[1]);
+              result.addAttribute("simd_modifier", mod);
+            }


Is the formatting off here?

I think this is not addressed.

kiranchandramohan

LGTM. I have one comment. Can be ignored if not necessary.

Can we upstream the mlir portion?

kiranchandramohan · 2021-07-21T13:15:51Z

flang/lib/Lower/OpenMP.cpp

+}
+
+static mlir::omp::ScheduleModifier
+getSIMDModifier(const Fortran::parser::OmpScheduleClause &x) {


Can this function be combined with getScheduleModifier?

The implementation supports static schedule for Fortran do loops. This implements the dynamic variant of the same concept. Reviewed By: Meinersbur Differential Revision: https://reviews.llvm.org/D97393

When using parallel loop construct, the OpenMP specification allows for guided, auto and runtime as scheduling variants (as well as static and dynamic which are already supported). Reviewed By: jdoerfert Differential Revision: https://reviews.llvm.org/D101435

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. Differential Revision: https://reviews.llvm.org/D102008

Pass the modifiers from the Flang parser to FIR/MLIR workshare loop operation. Not yet supporting the SIMD modifier, which is a bit more work than just adding it to the list of modifiers, so will go in a separate patch. This adds a new field to the WsLoopOp. Also add test for dynamic WSLoop, checking that dynamic schedule calls the init and next functions as expected.

Add support for SIMD modifier in OpenMP worksharing loops.

schweitzpgi · 2021-07-22T16:21:49Z

LGTM. I have one comment. Can be ignored if not necessary.

Can we upstream the mlir portion?

In general, feel free to upstream any portions that make sense here.

Some OpenMP local changes related to SIMD were done in #791 and touched llvm parts outside of flang. These parts were lost in the rebase/cannot be applied anymore since the files changed too much. The upstreaming of these changes is ongoing in: https://reviews.llvm.org/D111051 Try as much as possible to apply D111051 in a way that works with fir-dev environment.

kiranchandramohan requested review from SouraVX, clementval, kiranchandramohan, jeanPerier and schweitzpgi May 21, 2021 13:52

schweitzpgi reviewed May 25, 2021

View reviewed changes

Leporacanthicus force-pushed the modifiers branch 2 times, most recently from 7ccb9b8 to 60d6c24 Compare May 26, 2021 17:06

jeanPerier force-pushed the fir-dev branch from a636458 to e771da8 Compare May 27, 2021 15:16

Leporacanthicus force-pushed the modifiers branch from 60d6c24 to c6057cc Compare June 10, 2021 17:27

kiranchandramohan requested changes Jun 14, 2021

View reviewed changes

Leporacanthicus force-pushed the modifiers branch from c6057cc to 241ed58 Compare June 24, 2021 16:59

kiranchandramohan reviewed Jul 6, 2021

View reviewed changes

Leporacanthicus force-pushed the modifiers branch 2 times, most recently from f3c5981 to 14b94be Compare July 12, 2021 11:07

kiranchandramohan approved these changes Jul 21, 2021

View reviewed changes

Leporacanthicus added 5 commits July 22, 2021 13:36

[OpenMP IRBuilder, MLIR] Add support for OpenMP do schedule dynamic

235e0f5

The implementation supports static schedule for Fortran do loops. This implements the dynamic variant of the same concept. Reviewed By: Meinersbur Differential Revision: https://reviews.llvm.org/D97393

[Flang][OpenMP] Add support for SIMD modifier

cba7222

Add support for SIMD modifier in OpenMP worksharing loops.

Leporacanthicus force-pushed the modifiers branch from 14b94be to cba7222 Compare July 22, 2021 13:27

schweitzpgi merged commit 18b0621 into flang-compiler:fir-dev Jul 23, 2021

[OpenMP][Flang]Add support for modifiers monotonic and non-monotonic #791

[OpenMP][Flang]Add support for modifiers monotonic and non-monotonic #791

Uh oh!

Conversation

Leporacanthicus commented May 21, 2021

Uh oh!

jeanPerier commented May 21, 2021

Uh oh!

schweitzpgi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeanPerier commented May 27, 2021

Uh oh!

Leporacanthicus commented Jun 2, 2021

Uh oh!

Leporacanthicus commented Jun 10, 2021

Uh oh!

kiranchandramohan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiranchandramohan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schweitzpgi commented Jul 22, 2021

Uh oh!

Uh oh!