From e030006bd004c2e321e49f2b7d651771b82671d6 Mon Sep 17 00:00:00 2001 From: Nimish Mishra Date: Mon, 16 Jun 2025 13:47:10 +0530 Subject: [PATCH 1/2] [flang][OpenMP] Do not skip privatization of linear variable if it is OmpPreDetermined --- .../lib/Lower/OpenMP/DataSharingProcessor.cpp | 40 ++++++++++++++++++- 1 file changed, 38 insertions(+), 2 deletions(-) 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) { + // Use %linear in this BB + // } + // + // to be changed to the following: + // + // omp. op linear(%linear = %step : !fir.ref) + // private(%linear -> %arg0 : !fir.ref) { + // 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(&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); } From b23b03f538159462b3a6ddd449f8806f6ac936a0 Mon Sep 17 00:00:00 2001 From: NimishMishra <42909663+NimishMishra@users.noreply.github.com> Date: Thu, 19 Jun 2025 23:02:18 -0700 Subject: [PATCH 2/2] Update DataSharingProcessor.cpp --- flang/lib/Lower/OpenMP/DataSharingProcessor.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp index 1c50ba30ba40b..88b35fd344766 100644 --- a/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp +++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.cpp @@ -215,7 +215,7 @@ void DataSharingProcessor::collectSymbolsForPrivatization() { return false; // The handling of linear clause is deferred to the OpenMP - // IRBuilder which is responsible for all it's aspects, + // IRBuilder which is responsible for all its aspects, // including privatization. Privatizing linear variables at this point would // cause the following structure: //