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

ohno-fj · 2023-12-18T04:52:46Z

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
$

The text was updated successfully, but these errors were encountered:

llvmbot · 2023-12-18T04:53:01Z

@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 · 2023-12-18T10:30:47Z

@NimishMishra Does your fix for default cover this?

NimishMishra · 2023-12-21T07:14:58Z

@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 · 2023-12-21T11:14:02Z

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 · 2024-01-16T14:39:54Z

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 · 2024-04-30T21:51:50Z

#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.

When a critical construct is present inside another construct where privatizations may occur, such as a parallel construct, some privatizations are skipped if the corresponding symbols are defined inside the critical section only (see the example below). This happens because, while critical constructs have a "body", they don't have a separate scope (which makes sense, since no privatizations can occur in them). Because of this, in semantics phase, it's not possible to insert a new host association symbol, but instead the symbol from the enclosing context is used directly. This makes symbol collection in DataSharingProcessor consider the new symbol to be defined by the critical construct, instead of by the enclosing one, which causes the privatization to be skipped. Example: ``` !$omp parallel default(firstprivate) !$omp critical i = 200 !$omp end critical !$omp end parallel ``` This patch fixes this by identifying constructs where privatizations may not happen and skipping them during the collection of nested symbols. Currently, this seems to happen only with critical constructs, but others can be easily added to the skip list, if needed. Fixes llvm#75767

When a critical construct is present inside another construct where privatizations may occur, such as a parallel construct, some privatizations are skipped if the corresponding symbols are defined inside the critical section only (see the example below). This happens because, while critical constructs have a "body", they don't have a separate scope (which makes sense, since no privatizations can occur in them). Because of this, in semantics phase, it's not possible to insert a new host association symbol, but instead the symbol from the enclosing context is used directly. This makes symbol collection in DataSharingProcessor consider the new symbol to be defined by the critical construct, instead of by the enclosing one, which causes the privatization to be skipped. Example: ``` !$omp parallel default(firstprivate) !$omp critical i = 200 !$omp end critical !$omp end parallel ``` This patch fixes this by identifying constructs where privatizations may not happen and skipping them during the collection of nested symbols. Currently, this seems to happen only with critical constructs, but others can be easily added to the skip list, if needed. Fixes #75767

llvmbot · 2024-06-06T13:57:49Z

@llvm/issue-subscribers-flang-ir

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
$

ohno-fj added openmp flang Flang issues not falling into any other category labels Dec 18, 2023

ohno-fj added this to Flang - issues from Fujitsu testsuite Dec 18, 2023

luporl moved this to Todo in Flang - issues from Fujitsu testsuite Dec 18, 2023

luporl mentioned this issue Apr 30, 2024

[Flang][OpenMP] Incorrect execution result of a do-variable in critical construct which exists in parallel construct with default(none) clause #78936

Closed

luporl mentioned this issue May 16, 2024

[flang][OpenMP] Try to unify induction var privatization for OMP regions. #91116

Merged

luporl self-assigned this Jun 5, 2024

luporl moved this from Todo to In Progress in Flang - issues from Fujitsu testsuite Jun 5, 2024

luporl mentioned this issue Jun 5, 2024

[flang][OpenMP] Fix privatization when critical is present #94441

Merged

luporl closed this as completed in #94441 Jun 6, 2024

github-project-automation bot moved this from In Progress to Done in Flang - issues from Fujitsu testsuite Jun 6, 2024

EugeneZelenko added flang:ir flang:openmp and removed openmp flang Flang issues not falling into any other category labels Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ohno-fj commented Dec 18, 2023

llvmbot commented Dec 18, 2023

kiranchandramohan commented Dec 18, 2023

NimishMishra commented Dec 21, 2023

kiranchandramohan commented Dec 21, 2023

NimishMishra commented Jan 16, 2024

luporl commented Apr 30, 2024

llvmbot commented Jun 6, 2024

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

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

Comments

ohno-fj commented Dec 18, 2023

llvmbot commented Dec 18, 2023

kiranchandramohan commented Dec 18, 2023

NimishMishra commented Dec 21, 2023

kiranchandramohan commented Dec 21, 2023

NimishMishra commented Jan 16, 2024

luporl commented Apr 30, 2024

llvmbot commented Jun 6, 2024