[OpenMP][flang]Lower NUM_THREADS clause for parallel construct

[SouraVX]

Summary

This patch reflects the work that can be upstreamed from PR(merged)
PR: https://github.com/flang-compiler/f18-llvm-project/pull/411

[clementval]

flang/unittests/Lower/OpenMPLoweringTest.cpp

86 ↗

(On Diff #292566)

What is the plan with those unit tests once the bridge and lit test will be upstream. As I said previously, I don't think those test are testing the actual lowering and the API they use is probably tested in the respective MLIR tests. Are we going to delete them once we have the lit one or do we have to maintain both?

[SouraVX]

flang/unittests/Lower/OpenMPLoweringTest.cpp

86 ↗

(On Diff #292566)

What is the plan with those unit tests once the bridge and lit test will be upstream. As I said previously, I don't think those test are testing the actual lowering and the API they use is probably tested in the respective MLIR tests.

The plan is to continue this way(if people have nothing against this (including you)). Later at some point when we have the bridge, all we have to do is switch the builder and integrate the API.
Doing all this in one-step(when bridge is up) doesn't sound good.

Are we going to delete them once we have the lit one or do we have to maintain both?

These are just unit-tests, can/should be extended when a new Op is introduced or lowered. There won't be much of a maintenance issue that I can think of.
But at the same time if community decides/thinks these won't add much value, once regression tests are up and running. Deleting them would be a trivial task, only one file, no associated dependencies and all.

[clementval]

flang/unittests/Lower/OpenMPLoweringTest.cpp

86 ↗

(On Diff #292566)

My biggest concern is that these tests are disconnected from the actual code in flang/lib/Lower/OpenMP.cpp. I think @jeanPerier raised some concern in the past about those tests as well. If others are happy with them I'll follow the crowd but I have a hard time understanding what those tests are bringing in.

[SouraVX]

flang/unittests/Lower/OpenMPLoweringTest.cpp

86 ↗

(On Diff #292566)

Thanks for bringing more audience. Let's make a decision here as to whether we want this or not.
If not, I'm okay with that :) too.
If we decide against it, I'll take the responsibility of the cleanup:)

[jeanPerier]

flang/unittests/Lower/OpenMPLoweringTest.cpp

86 ↗

(On Diff #292566)

I share @clementval opinion here. These tests do not verify lowering, they verify properties of the parse-tree and mlir and belong better there. Here, they may give fake confidence to someone modifying genOMP that he did not break it.

The best would be to test genOMP here, though I agree it is easier said than done since that would require a lot of boilerplate and be harder to maintain than lit tests while not necessarily bringing much more confidence.

IMHO, if you want to keep these tests, they should be moved under an "OpenMP" directory under https://github.com/llvm/llvm-project/tree/master/mlir/unittests/Dialect (but for the tests like EXPECT_EQ(barrierDirective.v, llvm::omp::Directive::OMPD_barrier); that are parse-tree unit tests and would not fit under mlir unit tests. But I feel you can just skip that part that the lit tests will better cover instead).

[SouraVX]

Thanks @jeanPerier for reviewing this!

Here's the things on my TODO list:

• I'll revise this revision removing this test.
• Then after the patch lands, I'll do a patch for removal of the unit-test(Would it be necessary to file a separate review ?, IMHO this can be without bothering anyone.)

Please, let me know if you guys have any suggestion or anything.

flang/unittests/Lower/OpenMPLoweringTest.cpp

86 ↗

(On Diff #292566)

I share @clementval opinion here.

Okay, Thanks for letting us know.

These tests do not verify lowering, they verify properties of the parse-tree and mlir and belong better there. Here, they may give fake confidence to someone modifying genOMP that he did not break it.

Agreed!

The best would be to test genOMP

We investigated this before putting these tests, cannot happen till the some missing pieces of Bridge are in place.

IMHO, if you want to keep these tests, they should be moved under an "OpenMP" directory under ..

Sounds good! but at-least out of the purview of this patch.

[SouraVX]

• Addressed comments by @jeanPerier and @clementval.
• Rebased to the tip.

[jeanPerier]

LGTM regarding my comment for the unit test.

[SouraVX]

Removed unit-test in be1197c403b22291e35cbc5e96788860ceabd40c

flang/lib/Lower/OpenMP.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "flang/Lower/OpenMP.h"
 #include "flang/Lower/Bridge.h"
 #include "flang/Lower/FIRBuilder.h"
 #include "flang/Lower/PFTBuilder.h"
 #include "flang/Parser/parse-tree.h"
+#include "flang/Semantics/tools.h"
 #include "mlir/Dialect/OpenMP/OpenMPDialect.h"
 #include "llvm/Frontend/OpenMP/OMPConstants.h"
 
 #define TODO() llvm_unreachable("not yet implemented")
 
 static void genOMP(Fortran::lower::AbstractConverter &absConv,
                    Fortran::lower::pft::Evaluation &eval,
                    const Fortran::parser::OpenMPSimpleStandaloneConstruct
genOMP(Fortran::lower::AbstractConverter &absConv,
   const auto &blockDirective =
       std::get<Fortran::parser::OmpBeginBlockDirective>(blockConstruct.t);
   const auto &parallelDirective =
       std::get<Fortran::parser::OmpBlockDirective>(blockDirective.t);
   if (parallelDirective.v == llvm::omp::OMPD_parallel) {
     auto &firOpBuilder = absConv.getFirOpBuilder();
     auto currentLocation = absConv.getCurrentLocation();
     auto insertPt = firOpBuilder.saveInsertionPoint();
+
+    // Clauses.
+    // FIXME: Add support for other clauses.
+    mlir::Value numThreads;
+
+    const auto &parallelOpClauseList =
+        std::get<Fortran::parser::OmpClauseList>(blockDirective.t);
+    for (const auto &clause : parallelOpClauseList.v) {
+      if (const auto &numThreadsClause =
+              std::get_if<Fortran::parser::OmpClause::NumThreads>(&clause.u)) {
+        // OMPIRBuilder expects `NUM_THREAD` clause as a `Value`.
+        numThreads = absConv.genExprValue(
+            *Fortran::semantics::GetExpr(numThreadsClause->v));
+      }
+    }
     llvm::ArrayRef<mlir::Type> argTy;
-    mlir::ValueRange range;
-    llvm::SmallVector<int32_t, 6> operandSegmentSizes(6 /*Size=*/,
-                                                      0 /*Value=*/);
-    // create and insert the operation.
+    Attribute defaultValue, procBindValue;
+    // Create and insert the operation.
+    // Create the Op with empty ranges for clauses that are yet to be lowered.
     auto parallelOp = firOpBuilder.create<mlir::omp::ParallelOp>(
-        currentLocation, argTy, range);
-    parallelOp.setAttr(mlir::omp::ParallelOp::getOperandSegmentSizeAttr(),
-                       firOpBuilder.getI32VectorAttr(operandSegmentSizes));
-    parallelOp.getRegion().push_back(new Block{});
+        currentLocation, argTy, Value(), numThreads,
+        defaultValue.dyn_cast_or_null<StringAttr>(), ValueRange(), ValueRange(),
+        ValueRange(), ValueRange(),
+        procBindValue.dyn_cast_or_null<StringAttr>());
+    firOpBuilder.createBlock(&parallelOp.getRegion());
     auto &block = parallelOp.getRegion().back();
     firOpBuilder.setInsertionPointToStart(&block);
-    // ensure the block is well-formed.
+    // Ensure the block is well-formed.
     firOpBuilder.create<mlir::omp::TerminatorOp>(currentLocation);
     firOpBuilder.restoreInsertionPoint(insertPt);
   }
 }
 
 void Fortran::lower::genOpenMPConstruct(
     Fortran::lower::AbstractConverter &absConv,
     Fortran::lower::pft::Evaluation &eval,