Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 20, 2025
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e030006
[flang][OpenMP] Do not skip privatization of linear variable if it is…
NimishMishra Jun 16, 2025
b23b03f
Update DataSharingProcessor.cpp
NimishMishra Jun 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 38 additions & 2 deletions flang/lib/Lower/OpenMP/DataSharingProcessor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Comment on lines +212 to +215
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// The handling of linear clause is deferred to the OpenMP
// IRBuilder which is responsible for all its 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)) {
Expand All @@ -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);
}

Expand Down
Loading