This is an archive of the discontinued LLVM Phabricator instance.

[flang][OpenMP] Lowering support for lastprivate on unstructured sections construct
ClosedPublic

Authored by NimishMishra on Feb 1 2023, 7:58 AM.

Details

Summary

This patch adds lowering support for lastprivate privatization on unstructured construct. Generic lastprivate lowering on sections involves creating a scf.if operation on the lexically last section block and updating the lastprivate variable inside it. However, this control flow is not needed if the section construct is lowered as an unstructured construct. Hence, this patch modifies the lowering control flow to not emit an unnecessary scf.if if the section block is lowered as an unstructured construct.

Depends on D133686

Diff Detail

Event Timeline

NimishMishra created this revision.Feb 1 2023, 7:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2023, 7:58 AM
NimishMishra requested review of this revision.Feb 1 2023, 7:58 AM

Rebase with main

Fix CI fails

Could you add a description in the summary or add a comment in the changed code on what this change does and how it fixes the issue?

NimishMishra edited the summary of this revision. (Show Details)Mar 9 2023, 6:24 AM

LG.

While looking at this patch I saw that there is some firstprivatisation that is happening with the lastprivate clause in those section constructs that are not the last section. I am not sure why this happens. This might not be a bug since the private values can have any value, but it is definitely unnecessary instructions.

flang/lib/Lower/OpenMP.cpp
113

Nit: Insert braces in the else part.

272–273

Nit: Add a comment on what is the location of lastPrivOp. Is it the end of the last block in the lexically last section construct?

Do you know why the same code does not work for the structured case? I believe we inserted the scf.if operation to avoid some issue, but it is not actually required.

This revision is now accepted and ready to land.Apr 19 2023, 3:00 PM

LG.

While looking at this patch I saw that there is some firstprivatisation that is happening with the lastprivate clause in those section constructs that are not the last section. I am not sure why this happens. This might not be a bug since the private values can have any value, but it is definitely unnecessary instructions.

Thanks.

I was just wondering, are you referring to the following IR generated by the following test case. In this case, shouldn't the "lexically last section block" apply on the lastprivate case? Like isn't firstprivate applicable on all section blocks? If that is so, that is why firstprivate code is being produced in each section block, while lastprivate code is just in the final block.

If this is wrong though, let me know and I will fix it on priority.

!$omp sections firstprivate(x) lastprivate(x)
      !$omp section
            x = x * 10
      !$omp section
            x = x + 1
!$omp end sections

and the generated IR

omp.sections   {
      omp.section {
        %1 = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFlastprivateEx"}
        %2 = fir.load %0 : !fir.ref<i32>
        fir.store %2 to %1 : !fir.ref<i32>
        omp.barrier
        %3 = fir.load %0 : !fir.ref<i32>
        fir.store %3 to %1 : !fir.ref<i32>
        %c10_i32 = arith.constant 10 : i32
        %4 = fir.load %1 : !fir.ref<i32>
        %5 = arith.muli %c10_i32, %4 : i32
        fir.store %5 to %1 : !fir.ref<i32>
        omp.terminator
      }

omp.section {
        %1 = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFlastprivateEx"}
        %2 = fir.load %0 : !fir.ref<i32>
        fir.store %2 to %1 : !fir.ref<i32>
        omp.barrier
        %true = arith.constant true
        %3 = fir.load %1 : !fir.ref<i32>
        %c1_i32 = arith.constant 1 : i32
        %4 = arith.addi %3, %c1_i32 : i32
        fir.store %4 to %1 : !fir.ref<i32>
        scf.if %true {
          %5 = fir.load %1 : !fir.ref<i32>
          fir.store %5 to %0 : !fir.ref<i32>
        }
        omp.terminator
      }
omp.terminator
    }

The issue need not block this patch.

I was referring to tests like the following:

subroutine lastprivate()
  integer :: x
  !$omp sections lastprivate(x)
  !$omp section
      x = x * 10
  !$omp section
      x = x + 10
  !$omp end sections
end subroutine

that generates IR like the following:

func.func @_QPlastprivate() {
  %0 = fir.alloca i32 {bindc_name = "x", uniq_name = "_QFlastprivateEx"}
  omp.sections   {
    omp.section {
      %1 = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFlastprivateEx"}
      %2 = fir.load %0 : !fir.ref<i32>
      fir.store %2 to %1 : !fir.ref<i32>
      %c10_i32 = arith.constant 10 : i32
      %3 = fir.load %1 : !fir.ref<i32>
      %4 = arith.muli %c10_i32, %3 : i32
      fir.store %4 to %1 : !fir.ref<i32>
      omp.terminator
    }
    omp.section {
      %1 = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFlastprivateEx"}
      %true = arith.constant true
      %2 = fir.load %1 : !fir.ref<i32>
      %c10_i32 = arith.constant 10 : i32
      %3 = arith.addi %2, %c10_i32 : i32
      fir.store %3 to %1 : !fir.ref<i32>
      fir.if %true {
        %4 = fir.load %1 : !fir.ref<i32>
        fir.store %4 to %0 : !fir.ref<i32>
      }
      omp.terminator
    }
    omp.terminator
  }
  return
}

Note the following IR for the first section that seems to do a firstprivatisation although firstprivate is not specified.

%1 = fir.alloca i32 {bindc_name = "x", pinned, uniq_name = "_QFlastprivateEx"}
%2 = fir.load %0 : !fir.ref<i32>
fir.store %2 to %1 : !fir.ref<i32>