Page MenuHomePhabricator

[flang][OpenMP] Handle private/firstprivate clauses on sections construct
ClosedPublic

Authored by NimishMishra on Aug 8 2022, 8:52 PM.

Details

Summary

This patch adds private/firstprivate support for sections construct. For a source like the following:

!$omp sections private(x) firstprivate(y)
   !$omp section
       <block of code>
   !$omp section
        <block of code>
!$omp end sections

...privatization proceeds to privatize x and y accordingly inside each of the generated omp.section operations.

Diff Detail

Event Timeline

NimishMishra created this revision.Aug 8 2022, 8:52 PM
Herald added a project: Restricted Project. · View Herald Transcript
NimishMishra requested review of this revision.Aug 8 2022, 8:52 PM
NimishMishra added inline comments.Aug 9 2022, 10:29 AM
flang/lib/Lower/OpenMP.cpp
1350–1351

One way to do this would be to reuse collectSymbolSet from https://reviews.llvm.org/D123930

peixin requested changes to this revision.Aug 13 2022, 2:21 AM
peixin added inline comments.
flang/lib/Lower/OpenMP.cpp
1350–1351

The TODO or collectSymbol is no necessary here since the evaluation has the nested information of the parser node.

1354

You can move the code in Bridge.cpp here by getting the clause through the parser node of sections construct using the following code:

auto sectionsConstruct = eval.parentConstruct->getIf<Fortran::parser::SectionsConstruct>();

Then, there is no need to add one argument to genOpenMPConstruct, which is really unexpected.

This revision now requires changes to proceed.Aug 13 2022, 2:21 AM
NimishMishra added inline comments.Aug 18 2022, 6:26 PM
flang/lib/Lower/OpenMP.cpp
1350–1351

I get your getIf<Fortran::parser::SectionsConstruct>(). I was not aware of this functionality. This is way cleaner than the current implementation.

However, I was thinking of using collectSymbols for an orthogonal problem. Consider the following:

!$omp sections private(x, y)
   !$omp section
       x = x + 1
   !$omp section
       y = y + 1
!$omp end sections

If you check the current generated FIR, both x and y are privatised for both omp.section operations. I wanted to avoid that. I was thinking that since y is not used in the first operation, why privatise it there. For this, I thought to use collectSymbols on omp.section operation, and privatise only the symbols which are present in both private() list and in the section operation.

Let me know what you think of the problem.

peixin added inline comments.Aug 18 2022, 7:37 PM
flang/lib/Lower/OpenMP.cpp
1350–1351

You are right. It's true that privatization is sometimes redundant. But it seems using collectSymbols will fail for the following case:

program main
  integer :: x
  !$omp sections private(x)
   !$omp section
       call sub()
  !$omp end sections
contains
  subroutine sub()
    x = 1
  end
end

It's OK to have unnecessary privatization since DCE optimization in MLIR or LLVM middle-end will remove it.

Addressed comments.

NimishMishra added inline comments.Aug 20 2022, 8:33 PM
flang/lib/Lower/OpenMP.cpp
224

@peixin I looked into this. Currently sections does not have lastprivate support. Fixing this data race problem shall come once that support is present. Can I take up adding support for lastprivate on sections?

In any case, I guess it would be better to do that in a separate patch.

peixin added inline comments.Aug 21 2022, 6:21 PM
flang/lib/Lower/OpenMP.cpp
204–207
224

Can I take up adding support for lastprivate on sections?

Sure. Feel free to take up the unhandled clauses on sections.

In any case, I guess it would be better to do that in a separate patch.

Cannot agree more. The single-function patch is preferred for review.

1335

Nit:

1339

Nit: highlight the construct name so that users can find the error location easily is something goes wrong.

1349

Add one comment here to describe that you only handle the private/firstprivate clause here and the privitization should be performed in section region. Not sure if there should be one FIXME for other clauses. Maybe it's better to do it in line 1335-1348 that add TODO for unhandled clauses? If so, you can add one comment there the privitization should not be done in creating SectionsOp.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
641 ↗(On Diff #454272)

There are plans performing privitization in MLIR. You can keep it for now. If the final decision is to doing privitization in lowering. The TODOs for all constructs can be removed.

NimishMishra marked 4 inline comments as done.
NimishMishra edited the summary of this revision. (Show Details)

Addressed comments.

NimishMishra added inline comments.Aug 25 2022, 6:01 PM
flang/lib/Lower/OpenMP.cpp
1334

Removed this as there is a TODO already inside the function. I only had added this TODO long back. The current TODO for reduction is much better.

1349

I checked this. Maybe there's no explicit need to add more TODOs in 1335-1348. Reason being that out of private/firstprivate/lastprivate/reduction/allocate/nowait, the first two are being handled in this patch and the last 3 have some handling there already. For lastprivate, privatizeVars has a TODO already mentioning that no support is present except for wsloop.

Fixed formatting issues

peixin accepted this revision.Aug 26 2022, 8:02 PM

LGTM. The CI failed. Please make sure the CI passed before landing.

flang/lib/Lower/OpenMP.cpp
1349

OK. Makes sense to me.

This revision is now accepted and ready to land.Aug 26 2022, 8:02 PM

Re-triggered build. CI passes now.