[Flang] Fix for a crash in OpenMP MAX intrinsic reduction

[kavitha-natarajan]

Summary

Crash while using OpenMP MAX intrinsic reduction when the reduction is implemented in the following way:

!$omp parallel
!$omp do reduction(max:x)
do i=1, 100
  if (y(i) .gt. x) x = y(i)
end do
!$omp end do
!$omp end parallel

Although it is benefitted to use MAX intrinsic in the loop, this is also supported as per the OpenMP specification. This patch fixes the crash in this case.

[kiranchandramohan]

Thanks @kavitha-natarajan for the patch.

Although it is benefitted to use MAX intrinsic in the loop, this is also supported as per the OpenMP specification. This patch fixes the crash in this case.

Is your point here that an OpenMP reduction operation should be generated in this case? Or that it should not crash but ignore the reduction? A pointer to the standard would be helpful.

[kavitha-natarajan]

In D146967#4224656, @kiranchandramohan wrote:

Thanks @kavitha-natarajan for the patch.

Although it is benefitted to use MAX intrinsic in the loop, this is also supported as per the OpenMP specification. This patch fixes the crash in this case.

Is your point here that an OpenMP reduction operation should be generated in this case? Or that it should not crash but ignore the reduction? A pointer to the standard would be helpful.

I missed to note that reduction is not generated for this loop. I suppose the standard says reduction has to be generated for this case as well. Please refer to section A.36 - Example A.36.1f at page#266 in OpenMP 3.1 specification document: https://www.openmp.org/wp-content/uploads/OpenMP3.1.pdf

"
The following example demonstrates the reduction clause (Section2.9.3.6 on page 103); note that some reductions can be expressed in the loop in several ways, as shown for the max and min reductions below:

[. . .]
!$OMP PARALLEL DO PRIVATE(I) SHARED(X, Y, N) REDUCTION(+:A) &
!$OMP& REDUCTION(IEOR:B) REDUCTION(MIN:C) REDUCTION(MAX:D)

DO I=1,N
  A = A + X(I)
  B = IEOR(B, Y(I))
  C = MIN(C, Y(I))
  IF (D < X(I)) D = X(I)
END DO

"

Please correct me if my understanding is wrong. Otherwise, this patch needs more work to generate reduction when the intrinsic is not used directly as in the case of MIN in the above example.

[kiranchandramohan]

In D146967#4226407, @kavitha-natarajan wrote:

In D146967#4224656, @kiranchandramohan wrote:

Thanks @kavitha-natarajan for the patch.

Although it is benefitted to use MAX intrinsic in the loop, this is also supported as per the OpenMP specification. This patch fixes the crash in this case.

Is your point here that an OpenMP reduction operation should be generated in this case? Or that it should not crash but ignore the reduction? A pointer to the standard would be helpful.

I missed to note that reduction is not generated for this loop. I suppose the standard says reduction has to be generated for this case as well. Please refer to section A.36 - Example A.36.1f at page#266 in OpenMP 3.1 specification document: https://www.openmp.org/wp-content/uploads/OpenMP3.1.pdf

"
The following example demonstrates the reduction clause (Section2.9.3.6 on page 103); note that some reductions can be expressed in the loop in several ways, as shown for the max and min reductions below:

[. . .]
!$OMP PARALLEL DO PRIVATE(I) SHARED(X, Y, N) REDUCTION(+:A) &
!$OMP& REDUCTION(IEOR:B) REDUCTION(MIN:C) REDUCTION(MAX:D)

DO I=1,N
  A = A + X(I)
  B = IEOR(B, Y(I))
  C = MIN(C, Y(I))
  IF (D < X(I)) D = X(I)
END DO

"

Please correct me if my understanding is wrong. Otherwise, this patch needs more work to generate reduction when the intrinsic is not used directly as in the case of MIN in the above example.

From what is written in the examples document, it seems like this is supported. Classic Flang also supports this. I wasn't able to find the relevant section in the Standards Document though. I think we can merge the fix to avoid the crash. But could you create a github issue to fix this properly by generating a reduction operation?

flang/test/Lower/OpenMP/wsloop-reduction-max.f90

69

Please add a !CHECK-NOT omp.reduction here.

flang/lib/Lower/OpenMP.cpp

if (const auto &reductionClause =
                      reductionVal.getUses()) {
                   if (auto loadOp = mlir::dyn_cast<fir::LoadOp>(
                           reductionValUse.getOwner())) {
                     mlir::Value loadVal = loadOp.getRes();
                     // Max is lowered as a compare -> select.
                     // Match the pattern here.
                     mlir::Operation *reductionOp =
                         findReductionChain(loadVal, &reductionVal);
+                    if (reductionOp == nullptr) continue;
                     assert(mlir::isa<mlir::arith::SelectOp>(reductionOp) &&
                            "Selection Op not found in reduction intrinsic");
                     mlir::Operation *compareOp =
                         getCompareFromReductionOp(reductionOp, loadVal);
                     updateReduction(compareOp, firOpBuilder, loadVal,
                                     reductionVal);
                   }
                 }

flang/test/Lower/OpenMP/wsloop-reduction-max.f90

subroutine reduction_max_real(y)
   !$omp parallel
   !$omp do reduction(max:x)
   do i=1, 100
     x = max(y(i), x)
   end do
   !$omp end do
   !$omp end parallel
   print *, x
+
+  !$omp parallel
+  !$omp do reduction(max:x)
+  do i=1, 100
+    if (y(i) .gt. x) x = y(i)
+  end do
+  !$omp end do
+  !$omp end parallel
+  print *, x
 end subroutine