[flang][OpenMP] fix predetermined privatization inside section

[tblah]

This now produces code equivalent to if there was an explicit private clause on the SECTIONS construct.

The problem was that each SECTION construct got its own DSP, which tried to privatize the same symbol for that SECTION. Privatization for SECTION(S) happens on the outer SECTION construct and so the outer construct's DSP should be shared.

Fixes #135108

[llvmbot]

@llvm/pr-subscribers-flang-fir-hlfir

@llvm/pr-subscribers-flang-openmp

Author: Tom Eccles (tblah)

Changes

This now produces code equivalent to if there was an explicit private clause on the SECTIONS construct.

The problem was that each SECTION construct got its own DSP, which tried to privatize the same symbol for that SECTION. Privatization for SECTION(S) happens on the outer SECTION construct and so the outer construct's DSP should be shared.

Fixes #135108

---

Full diff: https://github.com/llvm/llvm-project/pull/138159.diff

2 Files Affected:

• (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+12-2)
• (added) flang/test/Lower/OpenMP/sections-predetermined-private.f90 (+34)

diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index f099028c23323..ae28d5ddd1564 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -1057,6 +1057,11 @@ struct OpWithBodyGenInfo {
     return *this;
   }
 
+  OpWithBodyGenInfo &setSkipDspStep2(bool value) {
+    skipDspStep2 = value;
+    return *this;
+  }
+
   OpWithBodyGenInfo &setEntryBlockArgs(const EntryBlockArgs *value) {
     blockArgs = value;
     return *this;
@@ -1088,6 +1093,8 @@ struct OpWithBodyGenInfo {
   const List<Clause> *clauses = nullptr;
   /// [in] if provided, processes the construct's data-sharing attributes.
   DataSharingProcessor *dsp = nullptr;
+  /// [in] if true, skip DataSharingProcessor::processStep2
+  bool skipDspStep2 = false;
   /// [in] if provided, it is used to create the op's region entry block. It is
   /// overriden when a \see genRegionEntryCB is provided. This is only valid for
   /// operations implementing the \see mlir::omp::BlockArgOpenMPOpInterface.
@@ -1240,7 +1247,7 @@ static void createBodyOfOp(mlir::Operation &op, const OpWithBodyGenInfo &info,
     // loop (this may not make sense in production code, but a user could
     // write that and we should handle it).
     firOpBuilder.setInsertionPoint(term);
-    if (privatize) {
+    if (privatize && !info.skipDspStep2) {
       // DataSharingProcessor::processStep2() may create operations before/after
       // the one passed as argument. We need to treat loop wrappers and their
       // nested loop as a unit, so we need to pass the bottom level wrapper (if
@@ -2162,7 +2169,10 @@ genSectionsOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
         OpWithBodyGenInfo(converter, symTable, semaCtx, loc, nestedEval,
                           llvm::omp::Directive::OMPD_section)
             .setClauses(&sectionQueue.begin()->clauses)
-            .setEntryBlockArgs(&args),
+            .setEntryBlockArgs(&args)
+            .setDataSharingProcessor(&dsp)
+            // lastprivate is handled differently for SECTIONS, see below
+            .setSkipDspStep2(true),
         sectionQueue, sectionQueue.begin());
   }
 
diff --git a/flang/test/Lower/OpenMP/sections-predetermined-private.f90 b/flang/test/Lower/OpenMP/sections-predetermined-private.f90
new file mode 100644
index 0000000000000..9c2e2e127aa78
--- /dev/null
+++ b/flang/test/Lower/OpenMP/sections-predetermined-private.f90
@@ -0,0 +1,34 @@
+! RUN: %flang_fc1 -fopenmp -emit-hlfir -o - %s | FileCheck %s
+
+!$omp parallel sections
+!$omp section
+    do i = 1, 2
+    end do
+!$omp section
+    do i = 1, 2
+    end do
+!$omp end parallel sections
+end
+! CHECK-LABEL:   func.func @_QQmain() {
+! CHECK:           omp.parallel {
+! CHECK:             %[[VAL_3:.*]] = fir.alloca i32 {bindc_name = "i", pinned}
+! CHECK:             %[[VAL_4:.*]]:2 = hlfir.declare %[[VAL_3]] {uniq_name = "_QFEi"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+! CHECK:             omp.sections {
+! CHECK:               omp.section {
+! CHECK:                 %[[VAL_11:.*]]:2 = fir.do_loop %[[VAL_12:.*]] = %{{.*}} to %{{.*}} step %{{.*}} iter_args(%{{.*}} = %{{.*}} -> (index, i32) {
+! CHECK:                 }
+! CHECK:                 fir.store %[[VAL_11]]#1 to %[[VAL_4]]#0 : !fir.ref<i32>
+! CHECK:                 omp.terminator
+! CHECK:               }
+! CHECK:               omp.section {
+! CHECK:                 %[[VAL_25:.*]]:2 = fir.do_loop %[[VAL_26:.*]] = %{{.*}} to %{{.*}} step %{{.*}} iter_args(%{{.*}} = %{{.*}}) -> (index, i32) {
+! CHECK:                 }
+! CHECK:                 fir.store %[[VAL_25]]#1 to %[[VAL_4]]#0 : !fir.ref<i32>
+! CHECK:                 omp.terminator
+! CHECK:               }
+! CHECK:               omp.terminator
+! CHECK:             }
+! CHECK:             omp.terminator
+! CHECK:           }
+! CHECK:           return
+! CHECK:         }

[skatrak]

We already have a special case in DataSharingProcessor::processStep2() to skip last-privatization when called for an omp.sections op. Wouldn't it make sense to return early for omp.section there, so the logic for handling the section(s) special case is spread across just two functions?

[tblah]

Thanks that was a good idea.

[skatrak]

Thank you, LGTM!