[flang][OpenMP] Place the insertion point to the start of the block

[SouraVX]

Summary

After skeleton of the Parallel Op is created set the insertion point to start of the block. So that later CodeGen can proceed.

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

[jdoerfert]

Please use a proper commit message and subject. Linking some other discussion is fine but it should be self contained.

[SouraVX]

Please use a proper commit message and subject. Linking some other discussion is fine but it should be self contained.

Sorry, I should've done better. Now modified to convey the intent.

[jdoerfert]

This is not "codegen support" is it? That sounds like "we now support codegen of X". Maybe: "Fix XXX codegen insertion point placement"?

Also "Code inside a Parallel operation region is lowered into FIR Dialect" seem to not capture this, this is just the "overall phase" in which the code is executed, right?
I mean, what happens here is the insert point is moved from something to something. Can we add a sentence that says why the new location is better? Maybe add a test?

[SouraVX]

In D88221#2292669, @jdoerfert wrote:

This is not "codegen support" is it? That sounds like "we now support codegen of X". Maybe: "Fix XXX codegen insertion point placement"?

Unfortunately only a small chunk can be upstreamed, so some where context is getting lost. This PR(mentioned) does the entire "CodeGen".

Also "Code inside a Parallel operation region is lowered into FIR Dialect" seem to not capture this, this is just the "overall phase" in which the code is executed, right?
I mean, what happens here is the insert point is moved from something to something. Can we add a sentence that says why the new location is better? Maybe add a test?

Regression test capturing some statements if and print statements(with end-to-end functionally tested)has been added to PR. Again unfortunately those also can't be added here. Since Lowering Code still lives in downstream/upstream FIR dev.

[SouraVX]

Are you alluding, that this review should be renamed as some "Upstream XXXX code from PR" and nothing else ?

[jdoerfert]

In D88221#2292676, @SouraVX wrote:

Are you alluding, that this review should be renamed as some "Upstream XXXX code from PR" and nothing else ?

I'm saying the commit subject and message should describe the code change. Currently, the subject and message describe the PR (I guess) but not this commit.

In D88221#2292675, @SouraVX wrote:

In D88221#2292669, @jdoerfert wrote:

This is not "codegen support" is it? That sounds like "we now support codegen of X". Maybe: "Fix XXX codegen insertion point placement"?

Unfortunately only a small chunk can be upstreamed, so some where context is getting lost. This PR(mentioned) does the entire "CodeGen".

Also "Code inside a Parallel operation region is lowered into FIR Dialect" seem to not capture this, this is just the "overall phase" in which the code is executed, right?
I mean, what happens here is the insert point is moved from something to something. Can we add a sentence that says why the new location is better? Maybe add a test?

Regression test capturing some statements if and print statements(with end-to-end functionally tested)has been added to PR. Again unfortunately those also can't be added here. Since Lowering Code still lives in downstream/upstream FIR dev.

This is indeed unfortunate, to say the least.

[kiranchandramohan]

LGTM. Tests and other changes should be upstreamed when the bridge infrastructure becomes available.

flang/lib/Lower/OpenMP.cpp

genOMP(Fortran::lower::AbstractConverter &converter,
        const Fortran::parser::OpenMPBlockConstruct &blockConstruct) {
   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 = converter.getFirOpBuilder();
     auto currentLocation = converter.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) {
auto parallelOp = firOpBuilder.create<mlir::omp::ParallelOp>(
         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.
     firOpBuilder.create<mlir::omp::TerminatorOp>(currentLocation);
-    firOpBuilder.restoreInsertionPoint(insertPt);
+
+    // Place the insertion point to the start of the first block.
+    firOpBuilder.setInsertionPointToStart(&block);
   }
 }
 
 void Fortran::lower::genOpenMPConstruct(
     Fortran::lower::AbstractConverter &converter,
     Fortran::lower::pft::Evaluation &eval,
     const Fortran::parser::OpenMPConstruct &ompConstruct) {