[Flang][OpenMP] Fix conversion to LLVM IR for Fortran code with OpenMP SIMD directive and nested loop

[domada]

Summary

Flang cannot convert to LLVM IR the following Fortran code:

subroutine simdloop_with_nested_loop
integer :: i, j
integer :: a(10)
 !$OMP SIMD
  do i=1, 10
   do j=1, 10
     a(i) = i
    end do
  end do
  !$OMP END SIMD
end subroutine

Command:
bin/bbc simdloop_with_nested_loop.f90 -fopenmp -emit-fir -o - | bin/tco
Fails with the error message:
loc("<stdin>":16:10): error: failed to legalize operation 'cf.br'

This patch enables compilation of the test case with SIMD directive and nested loop.

[peixin]

flang/test/Lower/OpenMP/simd.f90

4 ↗

(On Diff #450809)

This is not expected. You should add the test in flang/test/Fir/convert-to-llvm-openmp-and-fir.fir.

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp

126

SectionsOp is already there. Why do you add it repeatedly?

[domada]

Scope of changes:

1. moved test from flang/test/Lower/OpenMP/simd.f90 to flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
2. removed duplicated addition of SectionsOp in populateOpenMPToLLVMConversionPatterns function

[domada]

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp

126

Copy-paste error.

[peixin]

LGTM. Please enable the CI passed before landing this patch. It seems the build bot stuck.

[kiranchandramohan]

Changes to MLIR code should ideally be tested with MLIR tree dialect tests. The test for this code should have been in https://github.com/llvm/llvm-project/blob/main/mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir.

flang/test/Fir/convert-to-llvm-openmp-and-fir.fir

-// RUN: fir-opt --split-input-file --fir-to-llvm-ir="target=aarch64-unknown-linux-gnu" %s | FileCheck %s
+// RUN: fir-opt --split-input-file --cfg-conversion --fir-to-llvm-ir="target=aarch64-unknown-linux-gnu" %s | FileCheck %s
 
 func.func @_QPsb1(%arg0: !fir.ref<i32> {fir.bindc_name = "n"}, %arg1: !fir.ref<!fir.array<?xi32>> {fir.bindc_name = "arr"}) {
   %c1_i64 = arith.constant 1 : i64
   %c1_i32 = arith.constant 1 : i32
   %0 = fir.alloca i32 {bindc_name = "i", uniq_name = "_QFsbEi"}
   omp.parallel  {
     %1 = fir.alloca i32 {adapt.valuebyref, pinned}
     %2 = fir.load %arg0 : !fir.ref<i32>

 // CHECK:   %[[ARR_I_REF:.*]] = llvm.getelementptr %[[ARR_REF]][%[[I_CSTYLE]]] : (!llvm.ptr<i32>, i64) -> !llvm.ptr<i32>
 // CHECK:   llvm.store %[[I1]], %[[ARR_I_REF]] : !llvm.ptr<i32>
 // CHECK: omp.yield
 // CHECK: }
 // CHECK: omp.terminator
 // CHECK: }
 // CHECK: llvm.return
 // CHECK: }
+
+func.func @_QPsimdloop_with_nested_loop() {
+  %0 = fir.alloca i32 {adapt.valuebyref}
+  %1 = fir.alloca !fir.array<10xi32> {bindc_name = "a", uniq_name = "_QFsimdloop_with_nested_loopEa"}
+  %2 = fir.alloca i32 {bindc_name = "i", uniq_name = "_QFsimdloop_with_nested_loopEi"}
+  %3 = fir.alloca i32 {bindc_name = "j", uniq_name = "_QFsimdloop_with_nested_loopEj"}
+  %c1_i32 = arith.constant 1 : i32
+  %c10_i32 = arith.constant 10 : i32
+  %c1_i32_0 = arith.constant 1 : i32
+  omp.simdloop   for  (%arg0) : i32 = (%c1_i32) to (%c10_i32) inclusive step (%c1_i32_0) {
+    fir.store %arg0 to %0 : !fir.ref<i32>
+    %c1_i32_1 = arith.constant 1 : i32
+    %4 = fir.convert %c1_i32_1 : (i32) -> index
+    %c10_i32_2 = arith.constant 10 : i32
+    %5 = fir.convert %c10_i32_2 : (i32) -> index
+    %c1 = arith.constant 1 : index
+    %6 = fir.do_loop %arg1 = %4 to %5 step %c1 -> index {
+      %8 = fir.convert %arg1 : (index) -> i32
+      fir.store %8 to %3 : !fir.ref<i32>
+      %9 = fir.load %0 : !fir.ref<i32>
+      %10 = fir.load %0 : !fir.ref<i32>
+      %11 = fir.convert %10 : (i32) -> i64
+      %c1_i64 = arith.constant 1 : i64
+      %12 = arith.subi %11, %c1_i64 : i64
+      %13 = fir.coordinate_of %1, %12 : (!fir.ref<!fir.array<10xi32>>, i64) -> !fir.ref<i32>
+      fir.store %9 to %13 : !fir.ref<i32>
+      %14 = arith.addi %arg1, %c1 : index
+      fir.result %14 : index
+    }
+    %7 = fir.convert %6 : (index) -> i32
+    fir.store %7 to %3 : !fir.ref<i32>
+    omp.yield
+  }
+  return
+}
+
+// CHECK-LABEL:   llvm.func @_QPsimdloop_with_nested_loop() {
+// CHECK:           %[[LOWER:.*]] = llvm.mlir.constant(1 : i32) : i32
+// CHECK:           %[[UPPER:.*]] = llvm.mlir.constant(10 : i32) : i32
+// CHECK:           %[[STEP:.*]] = llvm.mlir.constant(1 : i32) : i32
+// CHECK:           omp.simdloop   for  (%[[CNT:.*]]) : i32 = (%[[LOWER]]) to (%[[UPPER]]) inclusive step (%[[STEP]]) {
+// CHECK:             llvm.br ^bb1(%[[VAL_1:.*]], %[[VAL_2:.*]] : i64, i64)
+// CHECK:           ^bb1(%[[VAL_3:.*]]: i64, %[[VAL_4:.*]]: i64):
+// CHECK:             %[[VAL_5:.*]] = llvm.mlir.constant(0 : index) : i64
+// CHECK:             %[[VAL_6:.*]] = llvm.icmp "sgt" %[[VAL_4]], %[[VAL_5]] : i64
+// CHECK:             llvm.cond_br %[[VAL_6]], ^bb2, ^bb3
+// CHECK:           ^bb2:
+// CHECK:             llvm.br ^bb1(%[[VAL_7:.*]], %[[VAL_8:.*]] : i64, i64)
+// CHECK:           ^bb3:
+// CHECK:             omp.yield
+// CHECK:           }
+// CHECK:           llvm.return
+// CHECK:         }

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp

matchAndRewrite(omp::ReductionOp curOp, OpAdaptor adaptor,
     return success();
   }
 };
 } // namespace
 
 void mlir::configureOpenMPToLLVMConversionLegality(
     ConversionTarget &target, LLVMTypeConverter &typeConverter) {
   target.addDynamicallyLegalOp<mlir::omp::CriticalOp, mlir::omp::ParallelOp,
-                               mlir::omp::WsLoopOp, mlir::omp::MasterOp,
-                               mlir::omp::SectionsOp, mlir::omp::SingleOp>(
-      [&](Operation *op) {
-        return typeConverter.isLegal(&op->getRegion(0)) &&
-               typeConverter.isLegal(op->getOperandTypes()) &&
-               typeConverter.isLegal(op->getResultTypes());
-      });
+                               mlir::omp::WsLoopOp, mlir::omp::SimdLoopOp,
+                               mlir::omp::MasterOp, mlir::omp::SectionsOp,
+                               mlir::omp::SingleOp>([&](Operation *op) {
   target
       .addDynamicallyLegalOp<mlir::omp::AtomicReadOp, mlir::omp::AtomicWriteOp,
                              mlir::omp::FlushOp, mlir::omp::ThreadprivateOp>(
           [&](Operation *op) {
             return typeConverter.isLegal(op->getOperandTypes()) &&
                    typeConverter.isLegal(op->getResultTypes());
           });
   target.addDynamicallyLegalOp<mlir::omp::ReductionOp>([&](Operation *op) {
     return typeConverter.isLegal(op->getOperandTypes());
   });
 }
 
 void mlir::populateOpenMPToLLVMConversionPatterns(LLVMTypeConverter &converter,
                                                   RewritePatternSet &patterns) {
   patterns.add<
       ReductionOpConversion, RegionOpConversion<omp::CriticalOp>,
       RegionOpConversion<omp::MasterOp>, ReductionOpConversion,
       RegionOpConversion<omp::MasterOp>, RegionOpConversion<omp::ParallelOp>,
       RegionOpConversion<omp::WsLoopOp>, RegionOpConversion<omp::SectionsOp>,
-      RegionOpConversion<omp::SingleOp>,
+      RegionOpConversion<omp::SimdLoopOp>, RegionOpConversion<omp::SingleOp>,
       RegionLessOpWithVarOperandsConversion<omp::AtomicReadOp>,
       RegionLessOpWithVarOperandsConversion<omp::AtomicWriteOp>,
       RegionLessOpWithVarOperandsConversion<omp::FlushOp>,
       RegionLessOpWithVarOperandsConversion<omp::ThreadprivateOp>>(converter);
 }
 
 namespace {
 struct ConvertOpenMPToLLVMPass