[flang][OpenMP] Do not skip privatization of linear variable if it is OmpPreDetermined

[NimishMishra]

Current implementation of linear clause skips privatisation of all linear variables during the FIR generation phase, since linear variables are handled in their entirety by the OpenMP IRBuilder. However, "implicit" linear variables (like OmpPreDetermined) cannot be skipped, since FIR generation requires privatized symbols. This patch adds checks to skip the same.

Fixes #142935

[llvmbot]

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

Author: None (NimishMishra)

Changes

Current implementation of linear clause skips privatisation of all linear variables during the FIR generation phase, since linear variables are handled in their entirety by the OpenMP IRBuilder. However, "implicit" linear variables (like OmpPreDetermined) cannot be skipped, since FIR generation requires privatized symbols. This patch adds checks to skip the same.

Fixes #142935

---

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

1 Files Affected:

• (modified) flang/lib/Lower/OpenMP/DataSharingProcessor.cpp (+38-2)

diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
index 8b334d7a392ac..1c50ba30ba40b 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
@@ -204,6 +204,42 @@ void DataSharingProcessor::collectOmpObjectListSymbol(
 }
 
 void DataSharingProcessor::collectSymbolsForPrivatization() {
+  // Add checks here for exceptional cases where privatization is not
+  // needed and be deferred to a later phase (like OpenMP IRBuilder).
+  // Such cases are suggested to be clearly documented and explained
+  // instead of being silently skipped
+  auto isException = [&](const Fortran::semantics::Symbol *sym) -> bool {
+    // `OmpPreDetermined` symbols cannot be exceptions since
+    // their privatized symbols are heavily used in FIR.
+    if (sym->test(Fortran::semantics::Symbol::Flag::OmpPreDetermined))
+      return false;
+
+    // The handling of linear clause is deferred to the OpenMP
+    // IRBuilder which is responsible for all it's aspects,
+    // including privatization. Privatizing linear variables at this point would
+    // cause the following structure:
+    //
+    // omp.op linear(%linear = %step : !fir.ref<type>) {
+    //	Use %linear in this BB
+    // }
+    //
+    // to be changed to the following:
+    //
+    // omp. op linear(%linear = %step : !fir.ref<type>)
+    // 	private(%linear -> %arg0 : !fir.ref<i32>) {
+    //	Declare and use %arg0 in this BB
+    // }
+    //
+    // The OpenMP IRBuilder needs to map the linear MLIR value
+    // (i.e. %linear) to its `uses` in the BB to correctly
+    // implement the functionalities of linear clause. However,
+    // privatizing here disallows the IRBuilder to
+    // draw a relation between %linear and %arg0. Hence skip.
+    if (sym->test(Fortran::semantics::Symbol::Flag::OmpLinear))
+      return true;
+    return false;
+  };
+
   for (const omp::Clause &clause : clauses) {
     if (const auto &privateClause =
             std::get_if<omp::clause::Private>(&clause.u)) {
@@ -222,10 +258,10 @@ void DataSharingProcessor::collectSymbolsForPrivatization() {
   }
 
   // TODO For common blocks, add the underlying objects within the block. Doing
-  // so, we won't need to explicitely handle block objects (or forget to do
+  // so, we won't need to explicitly handle block objects (or forget to do
   // so).
   for (auto *sym : explicitlyPrivatizedSymbols)
-    if (!sym->test(Fortran::semantics::Symbol::Flag::OmpLinear))
+    if (!isException(sym))
       allPrivatizedSymbols.insert(sym);
 }
 

[NimishMishra]

Failed test is Flang :: Profile/gcc-flag-compatibility.f90. It is known to be failing for some time on the buildbots.

[tblah]

If FIR can privatize LINEAR symbols correctly, why are we using OpenMPIRBuilder? If it cannot (as the comment below suggests), why are we skipping these?

[NimishMishra]

While working on lowering and translation of linear clause, I observed that Flang Semantics marks OmpLinear as a symbol to be privatised, which collectSymbolsForPrivatization fetches and privatizes. But it does not add code for any of the other linear semantics: (1) adding linear step based on the loop iteration, (2) generating code for finalization based on the last loop iteration, (3) storing the final variable value to its actual storage in the final iteration, and (4) emitting synchronization barrier. These four tasks had to be newly implemented; I added relevant functionality in the IRBuilder for these. Also, tasks (2) and (3) require knowledge of %lastiter variable of the canonical loop; I am not sure if FIR has access to this or an equivalent.

I think it would have been possible to let the FIR lowering continue with privatization here, and pass a tuple (linear-var MLIR value, private-copy MLIR value) to the IRBuilder to allow it to operate upon the two. But I thought it to be cleaner that all functionality wrt. linear semantics to be contained within the IRBuilder (hence added a new class LinearClauseProcessor in #139386).

I can perhaps modify the comment to reflect that while the FIR can privatize the linear variables, it is cleaner to simply let IRBuilder do so.

[NimishMishra]

Plus, in case we privatize during FIR lowering and then delegate rest of the functionality to LinearClauseProcessor, we will have to document that LinearClauseProcessor (or the IRBuilder in general) expects the frontend to privatize the linear variable and relay the information to LinearClauseProcessor.

[tblah]

So your plan is to make implicitly LINEAR variables PRIVATE instead of LINEAR?

[tblah]

LGTM with #144883

[mrkajetanp]

LGTM