[NFC][OpenMP] Update simd loop collapse support description

[domada]

Summary

Simdloop collapse clause is supported in the same way as colllapse clause for worksharing loops.

[peixin]

nit: add NFC to the title.

flang/test/Lower/OpenMP/simd.f90

123 ↗

(On Diff #451822)

This is not one good test case for SIMD directive. Can you replace it with the one that can be vectorized?

[kiranchandramohan]

As far as possible we should try to have Flang and MLIR OpenMP dialect changes in separate patches.

I would recommend splitting this into two simple patches.

1. Testing the lowering of OpenMP dialect simdloop collapse to LLVM IR. There is an existing test case, simdloop_multi in the translation tests. You need to strengthen it minimally to check the occurrence of collapse. And adding the collapse information to the OpenMP dialect op description.

The relevant code for this came in the patch https://reviews.llvm.org/D118065.

2. Test the lowering from source to FIR+OpenMP simdloop with collapse. Add the relevant patch that enabled lowering of the collapse in simdloop. I think the patches that you have provided are not the right ones.

flang/test/Lower/OpenMP/simd.f90

109 ↗

(On Diff #451822)

All the above checks can either be with the same CHECK line or with the CHECK-SAME directive.

[domada]

Add only OpenMP specific changes. Update simd loop MLIR description and strengthen translation test.

[domada]

In D131674#3722172, @kiranchandramohan wrote:

As far as possible we should try to have Flang and MLIR OpenMP dialect changes in separate patches.

I would recommend splitting this into two simple patches.

1. Testing the lowering of OpenMP dialect simdloop collapse to LLVM IR. There is an existing test case, simdloop_multi in the translation tests. You need to strengthen it minimally to check the occurrence of collapse. And adding the collapse information to the OpenMP dialect op description.

The relevant code for this came in the patch https://reviews.llvm.org/D118065.

Done. I modified this review so that it contains only OpenMP specific changes.

2. Test the lowering from source to FIR+OpenMP simdloop with collapse. Add the relevant patch that enabled lowering of the collapse in simdloop. I think the patches that you have provided are not the right ones.

Will be done as the separate patch.

[domada]

@peixin
Thank you for your review. I will apply your remarks in the separate patch which will be Flang specific.

[kiranchandramohan]

Thanks @domada, this looks better. I have a comment about the way the test is written.

mlir/test/Target/LLVMIR/openmp-llvm.mlir

719–722

As you know checking for names is not always the best. Can you instead do something similar to the way the collapse test is written in D105706?

[domada]

Updated test: Do not rely on hardcoded labels. Check if collapsed loop bound is calculated.

[kiranchandramohan]

LGTM. Thanks for making the changes.

[domada]

Follow up review: https://reviews.llvm.org/D132023 . This review contains Fortran test with simd clause.

flang/test/Lower/OpenMP/simd.f90

109 ↗

(On Diff #451822)

Done

123 ↗

(On Diff #451822)

@peixin done: please look at https://reviews.llvm.org/D132023

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

let description = [{
     be executed concurrently using SIMD instructions).. The lower and upper
     bounds specify a half-open range: the range includes the lower bound but
     does not include the upper bound. If the `inclusive` attribute is specified
     then the upper bound is also included.
 
     The body region can contain any number of blocks. The region is terminated
     by "omp.yield" instruction without operands.
 
+    Collapsed loops are represented by the simd-loop having a list of indices,
+    bounds and steps where the size of the list is equal to the collapse value.
+
     When an if clause is present and evaluates to false, the preferred number of
     iterations to be executed concurrently is one, regardless of whether
     a simdlen clause is specified.
 
     When a simdlen clause is present, the preferred number of iterations to be
     executed concurrently is the value provided to the simdlen clause.
     ```
     omp.simdloop <clauses>

mlir/test/Target/LLVMIR/openmp-llvm.mlir

 
 // -----
 
 // CHECK-LABEL: @simdloop_simple_multiple
 llvm.func @simdloop_simple_multiple(%lb1 : i64, %ub1 : i64, %step1 : i64, %lb2 : i64, %ub2 : i64, %step2 : i64, %arg0: !llvm.ptr<f32>, %arg1: !llvm.ptr<f32>) {
   omp.simdloop for (%iv1, %iv2) : i64 = (%lb1, %lb2) to (%ub1, %ub2) step (%step1, %step2) {
     %3 = llvm.mlir.constant(2.000000e+00 : f32) : f32
     // The form of the emitted IR is controlled by OpenMPIRBuilder and
-    // tested there. Just check that the right metadata is added.
+    // tested there. Just check that the right metadata is added and collapsed
+    // blocks are generated (Collapse clause is represented as a loop with
+    // list of indices, bounds and steps where the size of the list is equal
+    // to the collapse value.)
+    // CHECK: omp_collapsed.preheader
+    // CHECK: omp_collapsed.header
+    // CHECK: omp_collapsed.cond
+    // CHECK: omp_collapsed.body
     // CHECK: llvm.access.group
     // CHECK-NEXT: llvm.access.group
     %4 = llvm.getelementptr %arg0[%iv1] : (!llvm.ptr<f32>, i64) -> !llvm.ptr<f32>
     %5 = llvm.getelementptr %arg1[%iv2] : (!llvm.ptr<f32>, i64) -> !llvm.ptr<f32>
     llvm.store %3, %4 : !llvm.ptr<f32>
     llvm.store %3, %5 : !llvm.ptr<f32>
     omp.yield
   }