[Flang][OpenMP] Incorrect execution result of a variable in critical construct in parallel construct with default(firstprivate) clause

[ohno-fj]

Version of flang-new : 18.0.0(76bbbcb41bcf4a1d7a26bb11b78cf97b60ea7d4b)

The execution result of a variable in critical construct in parallel construct with default(firstprivate) clause is incorrect.
The execution result is correct if:

• remove critical construct, or
• change default(firstprivate) to firstprivate(j)

The following are the test program, Flang-new, Gfortran and ifort compilation/execution result.

snf_ng_01_1.f90:

program main
  INTEGER :: j
  j=2
!$OMP PARALLEL default(firstprivate)
!$OMP critical
  j=200
!$OMP END critical
!$OMP END PARALLEL
  if (j.ne.2) print *,'err var j=',j
  print *,'pass'
end program main

$ export OMP_NUM_THREADS=2; flang-new -fopenmp snf_ng_01_1.f90; ./a.out
 err var j= 200
 pass
$

$ export OMP_NUM_THREADS=2; gfortran -fopenmp snf_ng_01_1.f90; ./a.out
 pass
$

$ export OMP_NUM_THREADS=2; ifort -qopenmp snf_ng_01_1.f90; ./a.out
 pass
$

[llvmbot]

@llvm/issue-subscribers-openmp

Author: None (ohno-fj)

``` Version of flang-new : 18.0.0(76bbbcb) ```

The execution result of a variable in critical construct in parallel construct with default(firstprivate) clause is incorrect.
The execution result is correct if:

• remove critical construct, or
• change default(firstprivate) to firstprivate(j)

The following are the test program, Flang-new, Gfortran and ifort compilation/execution result.

snf_ng_01_1.f90:

program main
  INTEGER :: j
  j=2
!$OMP PARALLEL default(firstprivate)
!$OMP critical
  j=200
!$OMP END critical
!$OMP END PARALLEL
  if (j.ne.2) print *,'err var j=',j
  print *,'pass'
end program main

$ export OMP_NUM_THREADS=2; flang-new -fopenmp snf_ng_01_1.f90; ./a.out
 err var j= 200
 pass
$

$ export OMP_NUM_THREADS=2; gfortran -fopenmp snf_ng_01_1.f90; ./a.out
 pass
$

$ export OMP_NUM_THREADS=2; ifort -qopenmp snf_ng_01_1.f90; ./a.out
 pass
$

[kiranchandramohan]

@NimishMishra Does your fix for default cover this?

[NimishMishra]

@kiranchandramohan I took an initial look at the ticket, and it seems like the issue isn't default lowering.

To be able to lower default clause, in semantics phase, we add relevant symbols to parser::Name objects at void OmpAttributeVisitor::Post(const parser::Name &name) in https://github.com/llvm/llvm-project/blob/d5c98e783779c4e6b26b2010b20cd0ab7210ead3/flang/lib/Semantics/resolve-directives.cpp#L1902C1-L1902C57.

I am observing that symbol && !dirContext_.empty() && !GetContext().withinConstruct in this function is evaluated to True when parser::Name is within a critical construct (note the additional NOT for GetContext().withinConstruct which I have added). Transitively, the symbols needed for default clause are not being created. I'm not sure why it is happening though; need to investigate more. Are you aware of some previous issues similar to this?

Changing this directive to atomic or something else evaluates fine. So basically, GetContext().withinConstruct is evaluating to False for critical.

[kiranchandramohan]

withinConstruct is set to true for OpenMPBlockConstructs, OpenMPLoopConstructs while visiting their BeginDirectives. Atomic does not push a dirContext so withinConstruct does not apply here since it inherits it from the parent construct. Critical does not fit either of these and hence has this issue.

The various OpenMPConstructs are the following. Does the same issue affect OpenMPSectionsConstruct?

struct OpenMPConstruct {
  UNION_CLASS_BOILERPLATE(OpenMPConstruct);
  std::variant<OpenMPStandaloneConstruct, OpenMPSectionsConstruct,
      OpenMPSectionConstruct, OpenMPLoopConstruct, OpenMPBlockConstruct,
      OpenMPAtomicConstruct, OpenMPDeclarativeAllocate,
      OpenMPExecutableAllocate, OpenMPAllocatorsConstruct,
      OpenMPCriticalConstruct>

[NimishMishra]

withinConstruct is set to true for OpenMPBlockConstructs, OpenMPLoopConstructs while visiting their BeginDirectives. Atomic does not push a dirContext so withinConstruct does not apply here since it inherits it from the parent construct. Critical does not fit either of these and hence has this issue.

The various OpenMPConstructs are the following. Does the same issue affect OpenMPSectionsConstruct?

struct OpenMPConstruct {
  UNION_CLASS_BOILERPLATE(OpenMPConstruct);
  std::variant<OpenMPStandaloneConstruct, OpenMPSectionsConstruct,
      OpenMPSectionConstruct, OpenMPLoopConstruct, OpenMPBlockConstruct,
      OpenMPAtomicConstruct, OpenMPDeclarativeAllocate,
      OpenMPExecutableAllocate, OpenMPAllocatorsConstruct,
      OpenMPCriticalConstruct>

@kiranchandramohan This issue affects OpenMPSectionsConstruct too. I think we should be targeting fixing default lowering?

[luporl]

#90671 doesn't fix this issue, but it's a step forward, as it allows the symbol to be correctly processed in semantics phase.
Before:

!$omp parallel  default(firstprivate)
!$omp critical
 !REF: /main/j
 j = 200
!$omp end critical
!$omp end parallel

After:

!$omp parallel  default(firstprivate)
!$omp critical
 !DEF: /main/OtherConstruct1/j (OmpFirstPrivate) HostAssoc INTEGER(4)
 j = 200
!$omp end critical
!$omp end parallel

Lowering probably needs some tweaks as well.