Skip to content

[MLIR][OpenMP] Normalize representation of entry block arg-defining clauses #109809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

skatrak
Copy link
Member

@skatrak skatrak commented Sep 24, 2024

This patch updates printing and parsing of operations including clauses that define entry block arguments to the operation's region. This impacts in_reduction, map, private, reduction and task_reduction.

The proposed representation to be used by all such clauses is the following:

<clause_name>([byref] [@<sym>] %value -> %block_arg [, ...] : <type>[, ...]) {
  ...
}

The byref tag is only allowed for reduction-like clauses and the @<sym> is required and only allowed for the private and reduction-like clauses. The map clause does not accept any of these two.

This change fixes some currently broken op representations, like omp.teams or omp.sections reduction:

omp.teams reduction([byref] @<sym> -> %value : <type>) {
^bb0(%block_arg : <type>):
  ...
}

Additionally, it addresses some redundancy in the representation of the previously mentioned cases, as well as e.g. map in omp.target. The problem is that the block argument name after the arrow is not checked in any way, which makes some misleading representations legal:

omp.target map_entries(%x -> %arg1, %y -> %arg0, %z -> %doesnt_exist : !llvm.ptr, !llvm.ptr, !llvm.ptr) {
^bb0(%arg0 : !llvm.ptr, %arg1 : !llvm.ptr, %arg2 : !llvm.ptr):
  ...
}

In that case, %x maps to %arg0, contrary to what the representation states, and %z maps to %arg2. %doesnt_exist is not resolved, so it would likely cause issues if used anywhere inside of the operation's region.

The solution implemented in this patch makes it so that values introduced after the arrow on the representation of these clauses implicitly define the corresponding entry block arguments, removing the potential for these problematic representations. This is what is already implemented for the private and reduction clauses of omp.parallel.

There are a couple of consequences of this change:

  • Entry block argument-defining clauses must come at the end of the operation's representation and in alphabetical order. This is because they are printed/parsed as part of the region and a standardized ordering is needed to reliably match op arguments with their corresponding entry block arguments via the BlockArgOpenMPOpInterface.
  • We can no longer define per-clause assembly formats to be reused by all operations that take these clauses, since they must be passed to a custom printer including the region and arguments of all other entry block argument-defining clauses. Code duplication and potential for introducing issues is minimized by providing the generic {print,parse}BlockArgRegion helpers and associated structures.

MLIR and Flang lowering unit tests are updated due to changes in the order and formatting of impacted operations.

@llvmbot
Copy link
Member

llvmbot commented Sep 24, 2024

@llvm/pr-subscribers-flang-openmp
@llvm/pr-subscribers-mlir-llvm
@llvm/pr-subscribers-mlir-openmp

@llvm/pr-subscribers-flang-fir-hlfir

Author: Sergio Afonso (skatrak)

Changes

This patch updates printing and parsing of operations including clauses that define entry block arguments to the operation's region. This impacts in_reduction, map, private, reduction and task_reduction.

The proposed representation to be used by all such clauses is the following:

&lt;clause_name&gt;([byref] [@&lt;sym&gt;] %value -&gt; %block_arg [, ...] : &lt;type&gt;[, ...]) {
  ...
}

The byref tag is only allowed for reduction-like clauses and the @&lt;sym&gt; is required and only allowed for the private and reduction-like clauses. The map clause does not accept any of these two.

This change fixes some currently broken op representations, like omp.teams or omp.sections reduction:

omp.teams reduction([byref] @&lt;sym&gt; -&gt; %value : &lt;type&gt;) {
^bb0(%block_arg : &lt;type&gt;):
  ...
}

Additionally, it addresses some redundancy in the representation of the previously mentioned cases, as well as e.g. map in omp.target. The problem is that the block argument name after the arrow is not checked in any way, which makes some misleading representations legal:

omp.target map_entries(%x -&gt; %arg1, %y -&gt; %arg0, %z -&gt; %doesnt_exist : !llvm.ptr, !llvm.ptr, !llvm.ptr) {
^bb0(%arg0 : !llvm.ptr, %arg1 : !llvm.ptr, %arg2 : !llvm.ptr):
  ...
}

In that case, %x maps to %arg0, contrary to what the representation states, and %z maps to %arg2. %doesnt_exist is not resolved, so it would likely cause issues if used anywhere inside of the operation's region.

The solution implemented in this patch makes it so that values introduced after the arrow on the representation of these clauses implicitly define the corresponding entry block arguments, removing the potential for these problematic representations. This is what is already implemented for the private and reduction clauses of omp.parallel.

There are a couple of consequences of this change:

  • Entry block argument-defining clauses must come at the end of the operation's representation and in alphabetical order. This is because they are printed/parsed as part of the region and a standardized ordering is needed to reliably match op arguments with their corresponding entry block arguments via the BlockArgOpenMPOpInterface.
  • We can no longer define per-clause assembly formats to be reused by all operations that take these clauses, since they must be passed to a custom printer including the region and arguments of all other entry block argument-defining clauses. Code duplication and potential for introducing issues is minimized by providing the generic {print,parse}BlockArgRegion helpers and associated structures.

MLIR and Flang lowering unit tests are updated due to changes in the order and formatting of impacted operations.


Patch is 237.16 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/109809.diff

78 Files Affected:

  • (modified) flang/test/Fir/convert-to-llvm-openmp-and-fir.fir (+1-11)
  • (modified) flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90 (+7-6)
  • (modified) flang/test/Lower/OpenMP/DelayedPrivatization/target-private-simple.f90 (+1-2)
  • (modified) flang/test/Lower/OpenMP/Todo/omp-default-clause-inner-loop.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/common-block-map.f90 (-3)
  • (modified) flang/test/Lower/OpenMP/default-clause-byref.f90 (+15-15)
  • (modified) flang/test/Lower/OpenMP/default-clause.f90 (+18-18)
  • (modified) flang/test/Lower/OpenMP/delayed-privatization-private-firstprivate.f90 (+3-2)
  • (modified) flang/test/Lower/OpenMP/derived-type-map.f90 (-9)
  • (modified) flang/test/Lower/OpenMP/distribute-parallel-do-simd.f90 (+2-2)
  • (modified) flang/test/Lower/OpenMP/distribute-parallel-do.f90 (+2-2)
  • (modified) flang/test/Lower/OpenMP/firstprivate-commonblock.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/hlfir-seqloop-parallel.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/implicit-dsa.f90 (+3-3)
  • (modified) flang/test/Lower/OpenMP/map-component-ref.f90 (-1)
  • (modified) flang/test/Lower/OpenMP/parallel-firstprivate-clause-scalar.f90 (+5-5)
  • (modified) flang/test/Lower/OpenMP/parallel-private-clause.f90 (+4-4)
  • (modified) flang/test/Lower/OpenMP/parallel-reduction-add-byref.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/parallel-reduction-add.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/parallel-wsloop.f90 (+2-2)
  • (modified) flang/test/Lower/OpenMP/private-commonblock.f90 (+3-3)
  • (modified) flang/test/Lower/OpenMP/sections-array-reduction.f90 (+1-2)
  • (modified) flang/test/Lower/OpenMP/sections-reduction.f90 (+2-4)
  • (modified) flang/test/Lower/OpenMP/statement-function.f90 (+2-2)
  • (modified) flang/test/Lower/OpenMP/target.f90 (+1-8)
  • (modified) flang/test/Lower/OpenMP/unstructured.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-add-byref.f90 (+3-3)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-add.f90 (+3-3)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-logical-and-byref.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-logical-and.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-logical-eqv-byref.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-logical-eqv.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-logical-neqv-byref.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-logical-neqv.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-logical-or-byref.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-logical-or.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-mul-byref.f90 (+3-3)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-mul.f90 (+3-3)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-multi.f90 (+4-3)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-multiple-clauses.f90 (+1-1)
  • (modified) flang/test/Transforms/omp-map-info-finalization.fir (-6)
  • (modified) mlir/include/mlir/Dialect/OpenMP/OpenMPClauses.td (+15-25)
  • (modified) mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td (+75-24)
  • (modified) mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp (+412-266)
  • (modified) mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir (+1-5)
  • (modified) mlir/test/Dialect/OpenMP/invalid.mlir (+13-11)
  • (modified) mlir/test/Dialect/OpenMP/ops.mlir (+46-67)
  • (modified) mlir/test/Target/LLVMIR/omptarget-array-sectioning-host.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-byref-bycopy-generation-device.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-byref-bycopy-generation-host.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-constant-alloca-raise.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-constant-indexing-device-region.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-debug.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-debug2.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-declare-target-llvm-device.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-depend-host-only.mlir (+1-2)
  • (modified) mlir/test/Target/LLVMIR/omptarget-depend.mlir (+1-2)
  • (modified) mlir/test/Target/LLVMIR/omptarget-fortran-allocatable-types-host.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-fortran-common-block-host.mlir (-2)
  • (modified) mlir/test/Target/LLVMIR/omptarget-nested-record-type-mapping-host.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-parallel-llvm.mlir (-3)
  • (modified) mlir/test/Target/LLVMIR/omptarget-record-type-mapping-host.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-region-device-llvm.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-region-host-only.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-region-llvm.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-region-parallel-llvm.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-target-inside-task.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/openmp-data-target-device.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/openmp-parallel-reduction-cleanup.mlir (+1-1)
  • (modified) mlir/test/Target/LLVMIR/openmp-parallel-reduction-multiblock.mlir (+1-1)
  • (modified) mlir/test/Target/LLVMIR/openmp-private.mlir (+1-1)
  • (modified) mlir/test/Target/LLVMIR/openmp-reduction-array-sections.mlir (+1-2)
  • (modified) mlir/test/Target/LLVMIR/openmp-reduction-init-arg.mlir (+1-1)
  • (modified) mlir/test/Target/LLVMIR/openmp-reduction-sections.mlir (+1-2)
  • (modified) mlir/test/Target/LLVMIR/openmp-reduction.mlir (+3-3)
  • (modified) mlir/test/Target/LLVMIR/openmp-target-use-device-nested.mlir (+1-2)
  • (modified) mlir/test/Target/LLVMIR/openmp-task-target-device.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/openmp-wsloop-reduction-cleanup.mlir (+1-1)
diff --git a/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir b/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
index 4b9afd5675ea31..4d226eaa754c12 100644
--- a/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
+++ b/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
@@ -450,7 +450,6 @@ func.func @_QPomp_target() {
   %2 = omp.map.bounds   lower_bound(%c0 : index) upper_bound(%1 : index) extent(%c512 : index) stride(%c1 : index) start_idx(%c1 : index)
   %3 = omp.map.info var_ptr(%0 : !fir.ref<!fir.array<512xi32>>, !fir.array<512xi32>)   map_clauses(tofrom) capture(ByRef) bounds(%2) -> !fir.ref<!fir.array<512xi32>> {name = "a"}
   omp.target   thread_limit(%c64_i32 : i32) map_entries(%3 -> %arg0 : !fir.ref<!fir.array<512xi32>>) {
-    ^bb0(%arg0: !fir.ref<!fir.array<512xi32>>):
     %c10_i32 = arith.constant 10 : i32
     %c1_i64 = arith.constant 1 : i64
     %c1_i64_0 = arith.constant 1 : i64
@@ -472,8 +471,7 @@ func.func @_QPomp_target() {
 // CHECK:           %[[UPPER:.*]] = llvm.mlir.constant(511 : index) : i64
 // CHECK:           %[[BOUNDS:.*]] = omp.map.bounds   lower_bound(%[[LOWER]] : i64) upper_bound(%[[UPPER]] : i64) extent(%[[EXTENT]] : i64) stride(%[[STRIDE]] : i64) start_idx(%[[STRIDE]] : i64)
 // CHECK:           %[[MAP:.*]] = omp.map.info var_ptr(%[[VAL_1]] : !llvm.ptr, !llvm.array<512 x i32>)   map_clauses(tofrom) capture(ByRef) bounds(%[[BOUNDS]]) -> !llvm.ptr {name = "a"}
-// CHECK:           omp.target map_entries(%[[MAP]] -> %[[ARG_0:.*]] : !llvm.ptr) thread_limit(%[[VAL_2]] : i32) {
-// CHECK:           ^bb0(%[[ARG_0]]: !llvm.ptr):
+// CHECK:           omp.target thread_limit(%[[VAL_2]] : i32) map_entries(%[[MAP]] -> %[[ARG_0:.*]] : !llvm.ptr) {
 // CHECK:             %[[VAL_3:.*]] = llvm.mlir.constant(10 : i32) : i32
 // CHECK:             %[[VAL_4:.*]] = llvm.mlir.constant(1 : i64) : i64
 // CHECK:             %[[VAL_5:.*]] = llvm.mlir.constant(1 : i64) : i64
@@ -971,9 +969,7 @@ func.func @omp_map_info_derived_type_explicit_member_conversion(%arg0 : !fir.ref
   // CHECK: %[[MAP_PARENT:.*]] = omp.map.info var_ptr(%[[ARG_0]] : !llvm.ptr, !llvm.struct<"_QFderived_type", (f32, array<10 x i32>, i32)>) map_clauses(tofrom) capture(ByRef) members(%[[MAP_MEMBER_1]], %[[MAP_MEMBER_2]] : [2], [0] : !llvm.ptr, !llvm.ptr) -> !llvm.ptr {name = "dtype", partial_map = true} 
   %6 = omp.map.info var_ptr(%arg0 : !fir.ref<!fir.type<_QFderived_type{real:f32,array:!fir.array<10xi32>,int:i32}>>, !fir.type<_QFderived_type{real:f32,array:!fir.array<10xi32>,int:i32}>) map_clauses(tofrom) capture(ByRef) members(%2, %5 : [2], [0] : !fir.ref<i32>, !fir.ref<f32>) -> !fir.ref<!fir.type<_QFderived_type{real:f32,array:!fir.array<10xi32>,int:i32}>> {name = "dtype", partial_map = true}
   // CHECK: omp.target map_entries(%[[MAP_MEMBER_1]] -> %[[ARG_1:.*]], %[[MAP_MEMBER_2]] -> %[[ARG_2:.*]], %[[MAP_PARENT]] -> %[[ARG_3:.*]] : !llvm.ptr, !llvm.ptr, !llvm.ptr) {
-  // CHECK: ^bb0(%[[ARG_1]]: !llvm.ptr, %[[ARG_2]]: !llvm.ptr, %[[ARG_3]]: !llvm.ptr):
   omp.target map_entries(%2 -> %arg1, %5 -> %arg2, %6 -> %arg3 : !fir.ref<i32>, !fir.ref<f32>, !fir.ref<!fir.type<_QFderived_type{real:f32,array:!fir.array<10xi32>,int:i32}>>) {
-  ^bb0(%arg1: !fir.ref<f32>, %arg2: !fir.ref<i32>, %arg3: !fir.ref<!fir.type<_QFderived_type{real:f32,array:!fir.array<10xi32>,int:i32}>>):
     omp.terminator
   }
   return
@@ -1001,9 +997,7 @@ func.func @omp_map_info_nested_derived_type_explicit_member_conversion(%arg0 : !
     // CHECK: %[[PARENT_MAP:.*]] = omp.map.info var_ptr(%[[ARG_0]] : !llvm.ptr, !llvm.struct<"_QFTtop_layer", (array<10 x i32>, struct<"_QFTbottom_layer", (array<10 x f32>, f64)>, i32)>) map_clauses(tofrom) capture(ByRef) members(%[[MAP_MEMBER_1]], %[[MAP_MEMBER_2]] : [1,1], [2,-1] : !llvm.ptr, !llvm.ptr) -> !llvm.ptr {partial_map = true}
     %9 = omp.map.info var_ptr(%arg0 : !fir.ref<!fir.type<_QFTtop_layer{array_i:!fir.array<10xi32>,nested:!fir.type<_QFTbottom_layer{array_i2:!fir.array<10xf32>,i2:f64}>,k:i32}>>, !fir.type<_QFTtop_layer{array_i:!fir.array<10xi32>,nested:!fir.type<_QFTbottom_layer{array_i2:!fir.array<10xf32>,i2:f64}>,k:i32}>) map_clauses(tofrom) capture(ByRef) members(%4, %7 : [1,1], [2,-1] : !fir.ref<f64>, !fir.ref<i32>) -> !fir.ref<!fir.type<_QFTtop_layer{array_i:!fir.array<10xi32>,nested:!fir.type<_QFTbottom_layer{array_i2:!fir.array<10xf32>,i2:f64}>,k:i32}>> {partial_map = true}
     // CHECK: omp.target map_entries(%[[MAP_MEMBER_1]] -> %{{.*}}, %[[MAP_MEMBER_2]] -> %{{.*}}, %[[PARENT_MAP]] -> %{{.*}} : !llvm.ptr, !llvm.ptr, !llvm.ptr) {
-    // CHECK: ^bb0(%{{.*}}: !llvm.ptr, %{{.*}}: !llvm.ptr, %{{.*}}: !llvm.ptr):
     omp.target map_entries(%4 -> %arg1, %7 -> %arg2, %9 -> %arg3 : !fir.ref<f64>, !fir.ref<i32>, !fir.ref<!fir.type<_QFTtop_layer{array_i:!fir.array<10xi32>,nested:!fir.type<_QFTbottom_layer{array_i2:!fir.array<10xf32>,i2:f64}>,k:i32}>>) {
-     ^bb0(%arg1: !fir.ref<i32>, %arg2: !fir.ref<f64>, %arg3: !fir.ref<!fir.type<_QFTtop_layer{array_i:!fir.array<10xi32>,nested:!fir.type<_QFTbottom_layer{array_i2:!fir.array<10xf32>,i2:f64}>,k:i32}>>):
       omp.terminator
     }
   return
@@ -1016,7 +1010,6 @@ func.func @omp_map_info_nested_derived_type_explicit_member_conversion(%arg0 : !
 // CHECK: %[[ADDR_OF:.*]] = llvm.mlir.addressof @var_common_ : !llvm.ptr
 // CHECK: %[[CB_MAP:.*]] = omp.map.info var_ptr(%[[ADDR_OF]] : !llvm.ptr, !llvm.array<8 x i8>) map_clauses(tofrom) capture(ByRef) -> !llvm.ptr {name = "var_common"}
 // CHECK:    omp.target map_entries(%[[CB_MAP]] -> %[[ARG0:.*]] : !llvm.ptr) {
-// CHECK:    ^bb0(%[[ARG0]]: !llvm.ptr):
 // CHECK:      %[[VAR_2_OFFSET:.*]] = llvm.mlir.constant(4 : index) : i64
 // CHECK:      %[[VAR_1_OFFSET:.*]] = llvm.mlir.constant(0 : index) : i64
 // CHECK:      %{{.*}} = llvm.getelementptr %[[ARG0]][%[[VAR_1_OFFSET]]] : (!llvm.ptr, i64) -> !llvm.ptr, i8
@@ -1026,7 +1019,6 @@ func.func @omp_map_common_block_using_common_block_symbol() {
   %0 = fir.address_of(@var_common_) : !fir.ref<!fir.array<8xi8>>
   %1 = omp.map.info var_ptr(%0 : !fir.ref<!fir.array<8xi8>>, !fir.array<8xi8>) map_clauses(tofrom) capture(ByRef) -> !fir.ref<!fir.array<8xi8>> {name = "var_common"}
   omp.target map_entries(%1 -> %arg0 : !fir.ref<!fir.array<8xi8>>) {
-  ^bb0(%arg0: !fir.ref<!fir.array<8xi8>>):
     %c4 = arith.constant 4 : index
     %c0 = arith.constant 0 : index
     %c20_i32 = arith.constant 20 : i32
@@ -1058,7 +1050,6 @@ fir.global common @var_common_(dense<0> : vector<8xi8>) {alignment = 4 : i64} :
 // CHECK:    %[[MAP_CB_VAR_1:.*]] = omp.map.info var_ptr(%[[VAR_1_CB_GEP]] : !llvm.ptr, i32) map_clauses(tofrom) capture(ByRef) -> !llvm.ptr {name = "var1"}
 // CHECK:    %[[MAP_CB_VAR_2:.*]] = omp.map.info var_ptr(%[[VAR_2_CB_GEP]] : !llvm.ptr, i32) map_clauses(tofrom) capture(ByRef) -> !llvm.ptr {name = "var2"}
 // CHECK:    omp.target map_entries(%[[MAP_CB_VAR_1]] -> %[[ARG0:.*]], %[[MAP_CB_VAR_2]] -> %[[ARG1:.*]] : !llvm.ptr, !llvm.ptr) {
-// CHECK:     ^bb0(%[[ARG0]]: !llvm.ptr, %[[ARG1]]: !llvm.ptr):
 
 func.func @omp_map_common_block_using_common_block_members() {
   %c4 = arith.constant 4 : index
@@ -1073,7 +1064,6 @@ func.func @omp_map_common_block_using_common_block_members() {
   %7 = omp.map.info var_ptr(%3 : !fir.ref<i32>, i32) map_clauses(tofrom) capture(ByRef) -> !fir.ref<i32> {name = "var1"}
   %8 = omp.map.info var_ptr(%6 : !fir.ref<i32>, i32) map_clauses(tofrom) capture(ByRef) -> !fir.ref<i32> {name = "var2"}
   omp.target map_entries(%7 -> %arg0, %8 -> %arg1 : !fir.ref<i32>, !fir.ref<i32>) {
-  ^bb0(%arg0: !fir.ref<i32>, %arg1: !fir.ref<i32>):
     %c10_i32 = arith.constant 10 : i32
     %9 = fir.load %arg0 : !fir.ref<i32>
     %10 = arith.muli %9, %c10_i32 : i32
diff --git a/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90 b/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90
index 6e8282b2af6255..e3c1dc805d07b6 100644
--- a/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90
+++ b/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90
@@ -154,12 +154,13 @@ end subroutine target_allocatable
 ! CHECK:        omp.target
 ! CHECK-SAME:     map_entries(%[[MAPPED_MI]] -> %[[MAPPED_ARG:.*]] : !fir.ref<i32>)
 ! CHECK-SAME:     private(
-! CHECK-SAME:       @[[ALLOC_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[ALLOC_ARG:.*]] : !fir.ref<!fir.box<!fir.heap<i32>>>,
-! CHECK-SAME:       @[[REAL_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[REAL_ARG:.*]] : !fir.ref<f32>,
-! CHECK-SAME:       @[[LB_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[LB_ARG:.*]] : !fir.ref<i64>,
-! CHECK-SAME:       @[[ARR_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[ARR_ARG:.*]] : !fir.box<!fir.array<?xf32>>,
-! CHECK-SAME:       @[[COMP_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[COMP_ARG:.*]] : !fir.ref<!fir.complex<4>>,
-! CHECK-SAME:       @[[CHAR_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[CHAR_ARG:.*]] : !fir.boxchar<1>) {
+! CHECK-SAME:       @[[ALLOC_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[ALLOC_ARG:[^,]+]],
+! CHECK-SAME:       @[[REAL_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[REAL_ARG:[^,]+]],
+! CHECK-SAME:       @[[LB_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[LB_ARG:[^,]+]],
+! CHECK-SAME:       @[[ARR_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[ARR_ARG:[^,]+]],
+! CHECK-SAME:       @[[COMP_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[COMP_ARG:[^,]+]],
+! CHECK-SAME:       @[[CHAR_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[CHAR_ARG:[^,]+]] :
+! CHECK-SAME:       !fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<f32>, !fir.ref<i64>, !fir.box<!fir.array<?xf32>>, !fir.ref<!fir.complex<4>>, !fir.boxchar<1>) {
 ! CHECK-NOT:      fir.alloca
 ! CHECK:          hlfir.declare %[[MAPPED_ARG]]
 ! CHECK:          hlfir.declare %[[ALLOC_ARG]]
diff --git a/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-simple.f90 b/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-simple.f90
index 524e973780c49f..3c6836e81abe18 100644
--- a/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-simple.f90
+++ b/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-simple.f90
@@ -27,8 +27,7 @@ end subroutine target_simple
 ! CHECK:  %[[VAR_DECL:.*]]:2 = hlfir.declare %[[VAR_ALLOC]]
 
 ! CHECK:  omp.target private(
-! CHECK-SAME: @[[VAR_PRIVATIZER_SYM]] %[[VAR_DECL]]#0 -> %{{.*}} : !fir.ref<i32>) {
-! CHECK:    ^bb0(%[[REG_ARG:.*]]: !fir.ref<i32>):
+! CHECK-SAME: @[[VAR_PRIVATIZER_SYM]] %[[VAR_DECL]]#0 -> %[[REG_ARG:.*]] : !fir.ref<i32>) {
 ! CHECK:      %[[REG_DECL:.*]]:2 = hlfir.declare %[[REG_ARG]]
 ! CHECK:      %[[C10:.*]] = arith.constant 10
 ! CHECK:      hlfir.assign %[[C10]] to %[[REG_DECL]]#0
diff --git a/flang/test/Lower/OpenMP/Todo/omp-default-clause-inner-loop.f90 b/flang/test/Lower/OpenMP/Todo/omp-default-clause-inner-loop.f90
index a08cfc1a92e35f..42ebd37d1c4313 100644
--- a/flang/test/Lower/OpenMP/Todo/omp-default-clause-inner-loop.f90
+++ b/flang/test/Lower/OpenMP/Todo/omp-default-clause-inner-loop.f90
@@ -8,7 +8,7 @@
 
 ! The string "EXPECTED" denotes the expected FIR
 
-! CHECK: omp.parallel  private(@{{.*}} %{{.*}} -> %[[PRIVATE_Y:.*]] : !fir.ref<i32>, @{{.*}} %{{.*}} -> %[[PRIVATE_Y:.*]] : !fir.ref<i32>) {
+! CHECK: omp.parallel  private(@{{.*}} %{{.*}} -> %[[PRIVATE_Y:.*]], @{{.*}} %{{.*}} -> %[[PRIVATE_Y:.*]] : !fir.ref<i32>, !fir.ref<i32>) {
 ! CHECK: %[[TEMP:.*]] = fir.alloca i32 {bindc_name = "x", pinned, {{.*}}}
 ! CHECK: %[[const_1:.*]] = arith.constant 1 : i32
 ! CHECK: %[[const_2:.*]] = arith.constant 10 : i32
diff --git a/flang/test/Lower/OpenMP/common-block-map.f90 b/flang/test/Lower/OpenMP/common-block-map.f90
index 0c423efd5eef49..06df0d2d9fb18b 100644
--- a/flang/test/Lower/OpenMP/common-block-map.f90
+++ b/flang/test/Lower/OpenMP/common-block-map.f90
@@ -7,7 +7,6 @@
 !CHECK: %[[CB_ADDR:.*]] = fir.address_of(@var_common_) : !fir.ref<!fir.array<8xi8>>
 !CHECK: %[[MAP:.*]] = omp.map.info var_ptr(%[[CB_ADDR]] : !fir.ref<!fir.array<8xi8>>, !fir.array<8xi8>) map_clauses(tofrom) capture(ByRef) -> !fir.ref<!fir.array<8xi8>> {name = "var_common"}
 !CHECK: omp.target map_entries(%[[MAP]] -> %[[MAP_ARG:.*]] : !fir.ref<!fir.array<8xi8>>) {
-!CHECK:  ^bb0(%[[MAP_ARG]]: !fir.ref<!fir.array<8xi8>>):
 !CHECK:    %[[CONV:.*]] = fir.convert %[[MAP_ARG]] : (!fir.ref<!fir.array<8xi8>>) -> !fir.ref<!fir.array<?xi8>>
 !CHECK:    %[[INDEX:.*]] = arith.constant 0 : index
 !CHECK:    %[[COORD:.*]] = fir.coordinate_of %[[CONV]], %[[INDEX]] : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
@@ -43,7 +42,6 @@ subroutine map_full_block
 !CHECK: %[[MAP_EXP:.*]] = omp.map.info var_ptr(%[[CB_MEMBER_2]]#1 : !fir.ref<i32>, i32) map_clauses(tofrom) capture(ByRef) -> !fir.ref<i32> {name = "var2"}
 !CHECK: %[[MAP_IMP:.*]] = omp.map.info var_ptr(%[[CB_MEMBER_1]]#1 : !fir.ref<i32>, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !fir.ref<i32> {name = "var1"}
 !CHECK: omp.target map_entries(%[[MAP_EXP]] -> %[[ARG_EXP:.*]], %[[MAP_IMP]] -> %[[ARG_IMP:.*]] : !fir.ref<i32>, !fir.ref<i32>) {
-!CHECK: ^bb0(%[[ARG_EXP]]: !fir.ref<i32>, %[[ARG_IMP]]: !fir.ref<i32>):
 !CHECK:  %[[EXP_MEMBER:.*]]:2 = hlfir.declare %[[ARG_EXP]] {uniq_name = "_QFmap_mix_of_membersEvar2"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK:  %[[IMP_MEMBER:.*]]:2 = hlfir.declare %[[ARG_IMP]] {uniq_name = "_QFmap_mix_of_membersEvar1"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 subroutine map_mix_of_members
@@ -60,7 +58,6 @@ subroutine map_mix_of_members
 !CHECK: %[[DECL_TAR_CB:.*]] = fir.address_of(@var_common_link_) : !fir.ref<!fir.array<8xi8>>
 !CHECK: %[[MAP_DECL_TAR_CB:.*]] = omp.map.info var_ptr(%[[DECL_TAR_CB]] : !fir.ref<!fir.array<8xi8>>, !fir.array<8xi8>) map_clauses(tofrom) capture(ByRef) -> !fir.ref<!fir.array<8xi8>> {name = "var_common_link"}
 !CHECK: omp.target map_entries(%[[MAP_DECL_TAR_CB]] -> %[[MAP_DECL_TAR_ARG:.*]] : !fir.ref<!fir.array<8xi8>>) {
-!CHECK: ^bb0(%[[MAP_DECL_TAR_ARG]]: !fir.ref<!fir.array<8xi8>>):
 !CHECK:  %[[CONV:.*]] = fir.convert %[[MAP_DECL_TAR_ARG]] : (!fir.ref<!fir.array<8xi8>>) -> !fir.ref<!fir.array<?xi8>>
 !CHECK:  %[[INDEX:.*]] = arith.constant 0 : index
 !CHECK:  %[[COORD:.*]] = fir.coordinate_of %[[CONV]], %[[INDEX]] : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
diff --git a/flang/test/Lower/OpenMP/default-clause-byref.f90 b/flang/test/Lower/OpenMP/default-clause-byref.f90
index 7e9011f9c1bd56..6cdff407a97901 100644
--- a/flang/test/Lower/OpenMP/default-clause-byref.f90
+++ b/flang/test/Lower/OpenMP/default-clause-byref.f90
@@ -74,7 +74,7 @@
 !CHECK: %[[Z:.*]] = fir.alloca i32 {bindc_name = "z", uniq_name = "_QFEz"}
 !CHECK: %[[Z_DECL:.*]]:2 = hlfir.declare %[[Z]] {uniq_name = "_QFEz"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: omp.parallel private(
-!CHECK-SAME: @[[X_FIRSTPRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]] : {{.*}}, @[[Y_PRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]] : {{.*}}, @[[W_PRIVATIZER]] %[[W_DECL]]#0 -> %[[PRIVATE_W:.*]] : {{.*}}) {
+!CHECK-SAME: @[[X_FIRSTPRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]], @[[Y_PRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]], @[[W_PRIVATIZER]] %[[W_DECL]]#0 -> %[[PRIVATE_W:.*]] : {{.*}}) {
 !CHECK: %[[PRIVATE_X_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_X]] {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_Y_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_Y]] {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_W_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_W]] {uniq_name = "_QFEw"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
@@ -108,7 +108,7 @@ program default_clause_lowering
     !$omp end parallel
 
 !CHECK: omp.parallel private(
-!CHECK-SAME: @[[X_PRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]] : {{.*}}, @[[Y_PRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]] : {{.*}}) {
+!CHECK-SAME: @[[X_PRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]], @[[Y_PRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]] : {{.*}}) {
 !CHECK: %[[PRIVATE_X_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_X]] {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_Y_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_Y]] {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[TEMP:.*]] = fir.load %[[PRIVATE_Y_DECL]]#0 : !fir.ref<i32>
@@ -121,7 +121,7 @@ program default_clause_lowering
     !$omp end parallel
 
 !CHECK: omp.parallel private(
-!CHECK-SAME: @[[Y_FIRSTPRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]] : {{.*}}, @[[X_FIRSTPRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]] : {{.*}}) {
+!CHECK-SAME: @[[Y_FIRSTPRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]], @[[X_FIRSTPRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]] : {{.*}}) {
 !CHECK: %[[PRIVATE_Y_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_Y]] {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_X_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_X]] {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[TEMP:.*]] = fir.load %[[PRIVATE_Y_DECL]]#0 : !fir.ref<i32>
@@ -134,7 +134,7 @@ program default_clause_lowering
     !$omp end parallel
 
 !CHECK: omp.parallel private(
-!CHECK-SAME: @[[X_PRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]] : {{.*}}, @[[Y_FIRSTPRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]] : {{.*}}, @[[W_FIRSTPRIVATIZER]] %[[W_DECL]]#0 -> %[[PRIVATE_W:.*]] : {{.*}}) {
+!CHECK-SAME: @[[X_PRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]], @[[Y_FIRSTPRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]], @[[W_FIRSTPRIVATIZER]] %[[W_DECL]]#0 -> %[[PRIVATE_W:.*]] : {{.*}}) {
 !CHECK: %[[PRIVATE_X_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_X]] {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_Y_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_Y]] {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_W_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_W]] {uniq_name = "_QFEw"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
@@ -156,7 +156,7 @@ program default_clause_lowering
 
 !CHECK: omp.parallel   {
 !CHECK: omp.parallel   private(
-!CHECK-SAME: @[[X_PRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]] : {{.*}}, @[[Y_PRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]] : {{.*}}) {
+!CHECK-SAME: @[[X_PRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]], @[[Y_PRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]] : {{.*}}) {
 !CHECK: %[[PRIVATE_X_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_X]] {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_Y_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_Y]] {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[TEMP:.*]] = fir.load %[[PRIVATE_Y_DECL]]#0 : !fir.ref<i32>
@@ -164,7 +164,7 @@ program default_clause_lowering
 !CHECK: omp.terminator
 !CHECK: }
 !CHECK: omp.parallel private(
-!CHECK-SAME: @[[W_FIRSTPRIVATIZER]] %[[W_DECL]]#0 -> %[[PRIVATE_W:.*]] : {{.*}}, @[[X_FIRSTPRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]] : {{.*}}) {
+!CHECK-SAME: @[[W_FIRSTPRIVATIZER]] %[[W_DECL]]#0 -> %[[PRIVATE_W:.*]], @[[X_FIRSTPRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]] : {{.*}}) {
 !CHECK: %[[PRIVATE_W_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_W]] {uniq_name = "_QFEw"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_X_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_X]] {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[TEMP:.*]] = fir.load %[[PRIVATE_X_DECL]]#0 : !fir.ref<i32>
@@ -197,12 +197,12 @@ subroutine nested_default_clause_tests
 !CHECK: %[[Y_DECL:.*]]:2 = hlfir.declare %[[Y]] {uniq_name = "_QFnested_default_clause_testsEy"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[Z:.*]] = fir.alloca i32 {bindc_name = "z", uniq_name = "_QFnested_default_clause_testsEz"}
 !CHECK: %[[Z_DECL:.*]]:2 = hlfir.declare %[[Z]] {uniq_name = "_QFnested_default_clause_testsEz"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
-!CHECK: omp.parallel private({{.*firstprivate.*}} {{.*}}#0 -> %[[PRIVATE_X:.*]] : {{.*}}, {{.*}} {{.*}}#0 -> %[[PRIVATE_Y:.*]] : {{.*}}, {{.*}} {{.*}}#0 -> %[[PRIVATE_Z:.*]] : {{.*}}, {{.*}} {{.*}}#0 -> %[[PRIVATE_K:.*]] : {{.*}}) {
+!CHECK: omp.parallel private({{.*firstprivate.*}} {{.*}}#0 -> %[[PRIVATE_X:.*]], {{.*}} {{.*}}#0 -> %[[PRIVATE_Y:.*]], {{.*}} {{.*}}#0 -> %[[PRIVATE_Z:.*]], {{.*}} {{.*}}#0 -> %[[PRIVATE_K:.*]] : {{.*}}) {
 !CHECK: %[[PRIVATE_X_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_X...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Sep 24, 2024

@llvm/pr-subscribers-mlir

Author: Sergio Afonso (skatrak)

Changes

This patch updates printing and parsing of operations including clauses that define entry block arguments to the operation's region. This impacts in_reduction, map, private, reduction and task_reduction.

The proposed representation to be used by all such clauses is the following:

&lt;clause_name&gt;([byref] [@&lt;sym&gt;] %value -&gt; %block_arg [, ...] : &lt;type&gt;[, ...]) {
  ...
}

The byref tag is only allowed for reduction-like clauses and the @&lt;sym&gt; is required and only allowed for the private and reduction-like clauses. The map clause does not accept any of these two.

This change fixes some currently broken op representations, like omp.teams or omp.sections reduction:

omp.teams reduction([byref] @&lt;sym&gt; -&gt; %value : &lt;type&gt;) {
^bb0(%block_arg : &lt;type&gt;):
  ...
}

Additionally, it addresses some redundancy in the representation of the previously mentioned cases, as well as e.g. map in omp.target. The problem is that the block argument name after the arrow is not checked in any way, which makes some misleading representations legal:

omp.target map_entries(%x -&gt; %arg1, %y -&gt; %arg0, %z -&gt; %doesnt_exist : !llvm.ptr, !llvm.ptr, !llvm.ptr) {
^bb0(%arg0 : !llvm.ptr, %arg1 : !llvm.ptr, %arg2 : !llvm.ptr):
  ...
}

In that case, %x maps to %arg0, contrary to what the representation states, and %z maps to %arg2. %doesnt_exist is not resolved, so it would likely cause issues if used anywhere inside of the operation's region.

The solution implemented in this patch makes it so that values introduced after the arrow on the representation of these clauses implicitly define the corresponding entry block arguments, removing the potential for these problematic representations. This is what is already implemented for the private and reduction clauses of omp.parallel.

There are a couple of consequences of this change:

  • Entry block argument-defining clauses must come at the end of the operation's representation and in alphabetical order. This is because they are printed/parsed as part of the region and a standardized ordering is needed to reliably match op arguments with their corresponding entry block arguments via the BlockArgOpenMPOpInterface.
  • We can no longer define per-clause assembly formats to be reused by all operations that take these clauses, since they must be passed to a custom printer including the region and arguments of all other entry block argument-defining clauses. Code duplication and potential for introducing issues is minimized by providing the generic {print,parse}BlockArgRegion helpers and associated structures.

MLIR and Flang lowering unit tests are updated due to changes in the order and formatting of impacted operations.


Patch is 237.16 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/109809.diff

78 Files Affected:

  • (modified) flang/test/Fir/convert-to-llvm-openmp-and-fir.fir (+1-11)
  • (modified) flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90 (+7-6)
  • (modified) flang/test/Lower/OpenMP/DelayedPrivatization/target-private-simple.f90 (+1-2)
  • (modified) flang/test/Lower/OpenMP/Todo/omp-default-clause-inner-loop.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/common-block-map.f90 (-3)
  • (modified) flang/test/Lower/OpenMP/default-clause-byref.f90 (+15-15)
  • (modified) flang/test/Lower/OpenMP/default-clause.f90 (+18-18)
  • (modified) flang/test/Lower/OpenMP/delayed-privatization-private-firstprivate.f90 (+3-2)
  • (modified) flang/test/Lower/OpenMP/derived-type-map.f90 (-9)
  • (modified) flang/test/Lower/OpenMP/distribute-parallel-do-simd.f90 (+2-2)
  • (modified) flang/test/Lower/OpenMP/distribute-parallel-do.f90 (+2-2)
  • (modified) flang/test/Lower/OpenMP/firstprivate-commonblock.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/hlfir-seqloop-parallel.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/implicit-dsa.f90 (+3-3)
  • (modified) flang/test/Lower/OpenMP/map-component-ref.f90 (-1)
  • (modified) flang/test/Lower/OpenMP/parallel-firstprivate-clause-scalar.f90 (+5-5)
  • (modified) flang/test/Lower/OpenMP/parallel-private-clause.f90 (+4-4)
  • (modified) flang/test/Lower/OpenMP/parallel-reduction-add-byref.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/parallel-reduction-add.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/parallel-wsloop.f90 (+2-2)
  • (modified) flang/test/Lower/OpenMP/private-commonblock.f90 (+3-3)
  • (modified) flang/test/Lower/OpenMP/sections-array-reduction.f90 (+1-2)
  • (modified) flang/test/Lower/OpenMP/sections-reduction.f90 (+2-4)
  • (modified) flang/test/Lower/OpenMP/statement-function.f90 (+2-2)
  • (modified) flang/test/Lower/OpenMP/target.f90 (+1-8)
  • (modified) flang/test/Lower/OpenMP/unstructured.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-add-byref.f90 (+3-3)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-add.f90 (+3-3)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-logical-and-byref.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-logical-and.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-logical-eqv-byref.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-logical-eqv.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-logical-neqv-byref.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-logical-neqv.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-logical-or-byref.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-logical-or.f90 (+1-1)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-mul-byref.f90 (+3-3)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-mul.f90 (+3-3)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-multi.f90 (+4-3)
  • (modified) flang/test/Lower/OpenMP/wsloop-reduction-multiple-clauses.f90 (+1-1)
  • (modified) flang/test/Transforms/omp-map-info-finalization.fir (-6)
  • (modified) mlir/include/mlir/Dialect/OpenMP/OpenMPClauses.td (+15-25)
  • (modified) mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td (+75-24)
  • (modified) mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp (+412-266)
  • (modified) mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir (+1-5)
  • (modified) mlir/test/Dialect/OpenMP/invalid.mlir (+13-11)
  • (modified) mlir/test/Dialect/OpenMP/ops.mlir (+46-67)
  • (modified) mlir/test/Target/LLVMIR/omptarget-array-sectioning-host.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-byref-bycopy-generation-device.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-byref-bycopy-generation-host.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-constant-alloca-raise.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-constant-indexing-device-region.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-debug.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-debug2.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-declare-target-llvm-device.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-depend-host-only.mlir (+1-2)
  • (modified) mlir/test/Target/LLVMIR/omptarget-depend.mlir (+1-2)
  • (modified) mlir/test/Target/LLVMIR/omptarget-fortran-allocatable-types-host.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-fortran-common-block-host.mlir (-2)
  • (modified) mlir/test/Target/LLVMIR/omptarget-nested-record-type-mapping-host.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-parallel-llvm.mlir (-3)
  • (modified) mlir/test/Target/LLVMIR/omptarget-record-type-mapping-host.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-region-device-llvm.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-region-host-only.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-region-llvm.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-region-parallel-llvm.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/omptarget-target-inside-task.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/openmp-data-target-device.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/openmp-parallel-reduction-cleanup.mlir (+1-1)
  • (modified) mlir/test/Target/LLVMIR/openmp-parallel-reduction-multiblock.mlir (+1-1)
  • (modified) mlir/test/Target/LLVMIR/openmp-private.mlir (+1-1)
  • (modified) mlir/test/Target/LLVMIR/openmp-reduction-array-sections.mlir (+1-2)
  • (modified) mlir/test/Target/LLVMIR/openmp-reduction-init-arg.mlir (+1-1)
  • (modified) mlir/test/Target/LLVMIR/openmp-reduction-sections.mlir (+1-2)
  • (modified) mlir/test/Target/LLVMIR/openmp-reduction.mlir (+3-3)
  • (modified) mlir/test/Target/LLVMIR/openmp-target-use-device-nested.mlir (+1-2)
  • (modified) mlir/test/Target/LLVMIR/openmp-task-target-device.mlir (-1)
  • (modified) mlir/test/Target/LLVMIR/openmp-wsloop-reduction-cleanup.mlir (+1-1)
diff --git a/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir b/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
index 4b9afd5675ea31..4d226eaa754c12 100644
--- a/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
+++ b/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
@@ -450,7 +450,6 @@ func.func @_QPomp_target() {
   %2 = omp.map.bounds   lower_bound(%c0 : index) upper_bound(%1 : index) extent(%c512 : index) stride(%c1 : index) start_idx(%c1 : index)
   %3 = omp.map.info var_ptr(%0 : !fir.ref<!fir.array<512xi32>>, !fir.array<512xi32>)   map_clauses(tofrom) capture(ByRef) bounds(%2) -> !fir.ref<!fir.array<512xi32>> {name = "a"}
   omp.target   thread_limit(%c64_i32 : i32) map_entries(%3 -> %arg0 : !fir.ref<!fir.array<512xi32>>) {
-    ^bb0(%arg0: !fir.ref<!fir.array<512xi32>>):
     %c10_i32 = arith.constant 10 : i32
     %c1_i64 = arith.constant 1 : i64
     %c1_i64_0 = arith.constant 1 : i64
@@ -472,8 +471,7 @@ func.func @_QPomp_target() {
 // CHECK:           %[[UPPER:.*]] = llvm.mlir.constant(511 : index) : i64
 // CHECK:           %[[BOUNDS:.*]] = omp.map.bounds   lower_bound(%[[LOWER]] : i64) upper_bound(%[[UPPER]] : i64) extent(%[[EXTENT]] : i64) stride(%[[STRIDE]] : i64) start_idx(%[[STRIDE]] : i64)
 // CHECK:           %[[MAP:.*]] = omp.map.info var_ptr(%[[VAL_1]] : !llvm.ptr, !llvm.array<512 x i32>)   map_clauses(tofrom) capture(ByRef) bounds(%[[BOUNDS]]) -> !llvm.ptr {name = "a"}
-// CHECK:           omp.target map_entries(%[[MAP]] -> %[[ARG_0:.*]] : !llvm.ptr) thread_limit(%[[VAL_2]] : i32) {
-// CHECK:           ^bb0(%[[ARG_0]]: !llvm.ptr):
+// CHECK:           omp.target thread_limit(%[[VAL_2]] : i32) map_entries(%[[MAP]] -> %[[ARG_0:.*]] : !llvm.ptr) {
 // CHECK:             %[[VAL_3:.*]] = llvm.mlir.constant(10 : i32) : i32
 // CHECK:             %[[VAL_4:.*]] = llvm.mlir.constant(1 : i64) : i64
 // CHECK:             %[[VAL_5:.*]] = llvm.mlir.constant(1 : i64) : i64
@@ -971,9 +969,7 @@ func.func @omp_map_info_derived_type_explicit_member_conversion(%arg0 : !fir.ref
   // CHECK: %[[MAP_PARENT:.*]] = omp.map.info var_ptr(%[[ARG_0]] : !llvm.ptr, !llvm.struct<"_QFderived_type", (f32, array<10 x i32>, i32)>) map_clauses(tofrom) capture(ByRef) members(%[[MAP_MEMBER_1]], %[[MAP_MEMBER_2]] : [2], [0] : !llvm.ptr, !llvm.ptr) -> !llvm.ptr {name = "dtype", partial_map = true} 
   %6 = omp.map.info var_ptr(%arg0 : !fir.ref<!fir.type<_QFderived_type{real:f32,array:!fir.array<10xi32>,int:i32}>>, !fir.type<_QFderived_type{real:f32,array:!fir.array<10xi32>,int:i32}>) map_clauses(tofrom) capture(ByRef) members(%2, %5 : [2], [0] : !fir.ref<i32>, !fir.ref<f32>) -> !fir.ref<!fir.type<_QFderived_type{real:f32,array:!fir.array<10xi32>,int:i32}>> {name = "dtype", partial_map = true}
   // CHECK: omp.target map_entries(%[[MAP_MEMBER_1]] -> %[[ARG_1:.*]], %[[MAP_MEMBER_2]] -> %[[ARG_2:.*]], %[[MAP_PARENT]] -> %[[ARG_3:.*]] : !llvm.ptr, !llvm.ptr, !llvm.ptr) {
-  // CHECK: ^bb0(%[[ARG_1]]: !llvm.ptr, %[[ARG_2]]: !llvm.ptr, %[[ARG_3]]: !llvm.ptr):
   omp.target map_entries(%2 -> %arg1, %5 -> %arg2, %6 -> %arg3 : !fir.ref<i32>, !fir.ref<f32>, !fir.ref<!fir.type<_QFderived_type{real:f32,array:!fir.array<10xi32>,int:i32}>>) {
-  ^bb0(%arg1: !fir.ref<f32>, %arg2: !fir.ref<i32>, %arg3: !fir.ref<!fir.type<_QFderived_type{real:f32,array:!fir.array<10xi32>,int:i32}>>):
     omp.terminator
   }
   return
@@ -1001,9 +997,7 @@ func.func @omp_map_info_nested_derived_type_explicit_member_conversion(%arg0 : !
     // CHECK: %[[PARENT_MAP:.*]] = omp.map.info var_ptr(%[[ARG_0]] : !llvm.ptr, !llvm.struct<"_QFTtop_layer", (array<10 x i32>, struct<"_QFTbottom_layer", (array<10 x f32>, f64)>, i32)>) map_clauses(tofrom) capture(ByRef) members(%[[MAP_MEMBER_1]], %[[MAP_MEMBER_2]] : [1,1], [2,-1] : !llvm.ptr, !llvm.ptr) -> !llvm.ptr {partial_map = true}
     %9 = omp.map.info var_ptr(%arg0 : !fir.ref<!fir.type<_QFTtop_layer{array_i:!fir.array<10xi32>,nested:!fir.type<_QFTbottom_layer{array_i2:!fir.array<10xf32>,i2:f64}>,k:i32}>>, !fir.type<_QFTtop_layer{array_i:!fir.array<10xi32>,nested:!fir.type<_QFTbottom_layer{array_i2:!fir.array<10xf32>,i2:f64}>,k:i32}>) map_clauses(tofrom) capture(ByRef) members(%4, %7 : [1,1], [2,-1] : !fir.ref<f64>, !fir.ref<i32>) -> !fir.ref<!fir.type<_QFTtop_layer{array_i:!fir.array<10xi32>,nested:!fir.type<_QFTbottom_layer{array_i2:!fir.array<10xf32>,i2:f64}>,k:i32}>> {partial_map = true}
     // CHECK: omp.target map_entries(%[[MAP_MEMBER_1]] -> %{{.*}}, %[[MAP_MEMBER_2]] -> %{{.*}}, %[[PARENT_MAP]] -> %{{.*}} : !llvm.ptr, !llvm.ptr, !llvm.ptr) {
-    // CHECK: ^bb0(%{{.*}}: !llvm.ptr, %{{.*}}: !llvm.ptr, %{{.*}}: !llvm.ptr):
     omp.target map_entries(%4 -> %arg1, %7 -> %arg2, %9 -> %arg3 : !fir.ref<f64>, !fir.ref<i32>, !fir.ref<!fir.type<_QFTtop_layer{array_i:!fir.array<10xi32>,nested:!fir.type<_QFTbottom_layer{array_i2:!fir.array<10xf32>,i2:f64}>,k:i32}>>) {
-     ^bb0(%arg1: !fir.ref<i32>, %arg2: !fir.ref<f64>, %arg3: !fir.ref<!fir.type<_QFTtop_layer{array_i:!fir.array<10xi32>,nested:!fir.type<_QFTbottom_layer{array_i2:!fir.array<10xf32>,i2:f64}>,k:i32}>>):
       omp.terminator
     }
   return
@@ -1016,7 +1010,6 @@ func.func @omp_map_info_nested_derived_type_explicit_member_conversion(%arg0 : !
 // CHECK: %[[ADDR_OF:.*]] = llvm.mlir.addressof @var_common_ : !llvm.ptr
 // CHECK: %[[CB_MAP:.*]] = omp.map.info var_ptr(%[[ADDR_OF]] : !llvm.ptr, !llvm.array<8 x i8>) map_clauses(tofrom) capture(ByRef) -> !llvm.ptr {name = "var_common"}
 // CHECK:    omp.target map_entries(%[[CB_MAP]] -> %[[ARG0:.*]] : !llvm.ptr) {
-// CHECK:    ^bb0(%[[ARG0]]: !llvm.ptr):
 // CHECK:      %[[VAR_2_OFFSET:.*]] = llvm.mlir.constant(4 : index) : i64
 // CHECK:      %[[VAR_1_OFFSET:.*]] = llvm.mlir.constant(0 : index) : i64
 // CHECK:      %{{.*}} = llvm.getelementptr %[[ARG0]][%[[VAR_1_OFFSET]]] : (!llvm.ptr, i64) -> !llvm.ptr, i8
@@ -1026,7 +1019,6 @@ func.func @omp_map_common_block_using_common_block_symbol() {
   %0 = fir.address_of(@var_common_) : !fir.ref<!fir.array<8xi8>>
   %1 = omp.map.info var_ptr(%0 : !fir.ref<!fir.array<8xi8>>, !fir.array<8xi8>) map_clauses(tofrom) capture(ByRef) -> !fir.ref<!fir.array<8xi8>> {name = "var_common"}
   omp.target map_entries(%1 -> %arg0 : !fir.ref<!fir.array<8xi8>>) {
-  ^bb0(%arg0: !fir.ref<!fir.array<8xi8>>):
     %c4 = arith.constant 4 : index
     %c0 = arith.constant 0 : index
     %c20_i32 = arith.constant 20 : i32
@@ -1058,7 +1050,6 @@ fir.global common @var_common_(dense<0> : vector<8xi8>) {alignment = 4 : i64} :
 // CHECK:    %[[MAP_CB_VAR_1:.*]] = omp.map.info var_ptr(%[[VAR_1_CB_GEP]] : !llvm.ptr, i32) map_clauses(tofrom) capture(ByRef) -> !llvm.ptr {name = "var1"}
 // CHECK:    %[[MAP_CB_VAR_2:.*]] = omp.map.info var_ptr(%[[VAR_2_CB_GEP]] : !llvm.ptr, i32) map_clauses(tofrom) capture(ByRef) -> !llvm.ptr {name = "var2"}
 // CHECK:    omp.target map_entries(%[[MAP_CB_VAR_1]] -> %[[ARG0:.*]], %[[MAP_CB_VAR_2]] -> %[[ARG1:.*]] : !llvm.ptr, !llvm.ptr) {
-// CHECK:     ^bb0(%[[ARG0]]: !llvm.ptr, %[[ARG1]]: !llvm.ptr):
 
 func.func @omp_map_common_block_using_common_block_members() {
   %c4 = arith.constant 4 : index
@@ -1073,7 +1064,6 @@ func.func @omp_map_common_block_using_common_block_members() {
   %7 = omp.map.info var_ptr(%3 : !fir.ref<i32>, i32) map_clauses(tofrom) capture(ByRef) -> !fir.ref<i32> {name = "var1"}
   %8 = omp.map.info var_ptr(%6 : !fir.ref<i32>, i32) map_clauses(tofrom) capture(ByRef) -> !fir.ref<i32> {name = "var2"}
   omp.target map_entries(%7 -> %arg0, %8 -> %arg1 : !fir.ref<i32>, !fir.ref<i32>) {
-  ^bb0(%arg0: !fir.ref<i32>, %arg1: !fir.ref<i32>):
     %c10_i32 = arith.constant 10 : i32
     %9 = fir.load %arg0 : !fir.ref<i32>
     %10 = arith.muli %9, %c10_i32 : i32
diff --git a/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90 b/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90
index 6e8282b2af6255..e3c1dc805d07b6 100644
--- a/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90
+++ b/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90
@@ -154,12 +154,13 @@ end subroutine target_allocatable
 ! CHECK:        omp.target
 ! CHECK-SAME:     map_entries(%[[MAPPED_MI]] -> %[[MAPPED_ARG:.*]] : !fir.ref<i32>)
 ! CHECK-SAME:     private(
-! CHECK-SAME:       @[[ALLOC_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[ALLOC_ARG:.*]] : !fir.ref<!fir.box<!fir.heap<i32>>>,
-! CHECK-SAME:       @[[REAL_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[REAL_ARG:.*]] : !fir.ref<f32>,
-! CHECK-SAME:       @[[LB_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[LB_ARG:.*]] : !fir.ref<i64>,
-! CHECK-SAME:       @[[ARR_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[ARR_ARG:.*]] : !fir.box<!fir.array<?xf32>>,
-! CHECK-SAME:       @[[COMP_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[COMP_ARG:.*]] : !fir.ref<!fir.complex<4>>,
-! CHECK-SAME:       @[[CHAR_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[CHAR_ARG:.*]] : !fir.boxchar<1>) {
+! CHECK-SAME:       @[[ALLOC_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[ALLOC_ARG:[^,]+]],
+! CHECK-SAME:       @[[REAL_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[REAL_ARG:[^,]+]],
+! CHECK-SAME:       @[[LB_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[LB_ARG:[^,]+]],
+! CHECK-SAME:       @[[ARR_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[ARR_ARG:[^,]+]],
+! CHECK-SAME:       @[[COMP_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[COMP_ARG:[^,]+]],
+! CHECK-SAME:       @[[CHAR_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[CHAR_ARG:[^,]+]] :
+! CHECK-SAME:       !fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<f32>, !fir.ref<i64>, !fir.box<!fir.array<?xf32>>, !fir.ref<!fir.complex<4>>, !fir.boxchar<1>) {
 ! CHECK-NOT:      fir.alloca
 ! CHECK:          hlfir.declare %[[MAPPED_ARG]]
 ! CHECK:          hlfir.declare %[[ALLOC_ARG]]
diff --git a/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-simple.f90 b/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-simple.f90
index 524e973780c49f..3c6836e81abe18 100644
--- a/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-simple.f90
+++ b/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-simple.f90
@@ -27,8 +27,7 @@ end subroutine target_simple
 ! CHECK:  %[[VAR_DECL:.*]]:2 = hlfir.declare %[[VAR_ALLOC]]
 
 ! CHECK:  omp.target private(
-! CHECK-SAME: @[[VAR_PRIVATIZER_SYM]] %[[VAR_DECL]]#0 -> %{{.*}} : !fir.ref<i32>) {
-! CHECK:    ^bb0(%[[REG_ARG:.*]]: !fir.ref<i32>):
+! CHECK-SAME: @[[VAR_PRIVATIZER_SYM]] %[[VAR_DECL]]#0 -> %[[REG_ARG:.*]] : !fir.ref<i32>) {
 ! CHECK:      %[[REG_DECL:.*]]:2 = hlfir.declare %[[REG_ARG]]
 ! CHECK:      %[[C10:.*]] = arith.constant 10
 ! CHECK:      hlfir.assign %[[C10]] to %[[REG_DECL]]#0
diff --git a/flang/test/Lower/OpenMP/Todo/omp-default-clause-inner-loop.f90 b/flang/test/Lower/OpenMP/Todo/omp-default-clause-inner-loop.f90
index a08cfc1a92e35f..42ebd37d1c4313 100644
--- a/flang/test/Lower/OpenMP/Todo/omp-default-clause-inner-loop.f90
+++ b/flang/test/Lower/OpenMP/Todo/omp-default-clause-inner-loop.f90
@@ -8,7 +8,7 @@
 
 ! The string "EXPECTED" denotes the expected FIR
 
-! CHECK: omp.parallel  private(@{{.*}} %{{.*}} -> %[[PRIVATE_Y:.*]] : !fir.ref<i32>, @{{.*}} %{{.*}} -> %[[PRIVATE_Y:.*]] : !fir.ref<i32>) {
+! CHECK: omp.parallel  private(@{{.*}} %{{.*}} -> %[[PRIVATE_Y:.*]], @{{.*}} %{{.*}} -> %[[PRIVATE_Y:.*]] : !fir.ref<i32>, !fir.ref<i32>) {
 ! CHECK: %[[TEMP:.*]] = fir.alloca i32 {bindc_name = "x", pinned, {{.*}}}
 ! CHECK: %[[const_1:.*]] = arith.constant 1 : i32
 ! CHECK: %[[const_2:.*]] = arith.constant 10 : i32
diff --git a/flang/test/Lower/OpenMP/common-block-map.f90 b/flang/test/Lower/OpenMP/common-block-map.f90
index 0c423efd5eef49..06df0d2d9fb18b 100644
--- a/flang/test/Lower/OpenMP/common-block-map.f90
+++ b/flang/test/Lower/OpenMP/common-block-map.f90
@@ -7,7 +7,6 @@
 !CHECK: %[[CB_ADDR:.*]] = fir.address_of(@var_common_) : !fir.ref<!fir.array<8xi8>>
 !CHECK: %[[MAP:.*]] = omp.map.info var_ptr(%[[CB_ADDR]] : !fir.ref<!fir.array<8xi8>>, !fir.array<8xi8>) map_clauses(tofrom) capture(ByRef) -> !fir.ref<!fir.array<8xi8>> {name = "var_common"}
 !CHECK: omp.target map_entries(%[[MAP]] -> %[[MAP_ARG:.*]] : !fir.ref<!fir.array<8xi8>>) {
-!CHECK:  ^bb0(%[[MAP_ARG]]: !fir.ref<!fir.array<8xi8>>):
 !CHECK:    %[[CONV:.*]] = fir.convert %[[MAP_ARG]] : (!fir.ref<!fir.array<8xi8>>) -> !fir.ref<!fir.array<?xi8>>
 !CHECK:    %[[INDEX:.*]] = arith.constant 0 : index
 !CHECK:    %[[COORD:.*]] = fir.coordinate_of %[[CONV]], %[[INDEX]] : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
@@ -43,7 +42,6 @@ subroutine map_full_block
 !CHECK: %[[MAP_EXP:.*]] = omp.map.info var_ptr(%[[CB_MEMBER_2]]#1 : !fir.ref<i32>, i32) map_clauses(tofrom) capture(ByRef) -> !fir.ref<i32> {name = "var2"}
 !CHECK: %[[MAP_IMP:.*]] = omp.map.info var_ptr(%[[CB_MEMBER_1]]#1 : !fir.ref<i32>, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !fir.ref<i32> {name = "var1"}
 !CHECK: omp.target map_entries(%[[MAP_EXP]] -> %[[ARG_EXP:.*]], %[[MAP_IMP]] -> %[[ARG_IMP:.*]] : !fir.ref<i32>, !fir.ref<i32>) {
-!CHECK: ^bb0(%[[ARG_EXP]]: !fir.ref<i32>, %[[ARG_IMP]]: !fir.ref<i32>):
 !CHECK:  %[[EXP_MEMBER:.*]]:2 = hlfir.declare %[[ARG_EXP]] {uniq_name = "_QFmap_mix_of_membersEvar2"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK:  %[[IMP_MEMBER:.*]]:2 = hlfir.declare %[[ARG_IMP]] {uniq_name = "_QFmap_mix_of_membersEvar1"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 subroutine map_mix_of_members
@@ -60,7 +58,6 @@ subroutine map_mix_of_members
 !CHECK: %[[DECL_TAR_CB:.*]] = fir.address_of(@var_common_link_) : !fir.ref<!fir.array<8xi8>>
 !CHECK: %[[MAP_DECL_TAR_CB:.*]] = omp.map.info var_ptr(%[[DECL_TAR_CB]] : !fir.ref<!fir.array<8xi8>>, !fir.array<8xi8>) map_clauses(tofrom) capture(ByRef) -> !fir.ref<!fir.array<8xi8>> {name = "var_common_link"}
 !CHECK: omp.target map_entries(%[[MAP_DECL_TAR_CB]] -> %[[MAP_DECL_TAR_ARG:.*]] : !fir.ref<!fir.array<8xi8>>) {
-!CHECK: ^bb0(%[[MAP_DECL_TAR_ARG]]: !fir.ref<!fir.array<8xi8>>):
 !CHECK:  %[[CONV:.*]] = fir.convert %[[MAP_DECL_TAR_ARG]] : (!fir.ref<!fir.array<8xi8>>) -> !fir.ref<!fir.array<?xi8>>
 !CHECK:  %[[INDEX:.*]] = arith.constant 0 : index
 !CHECK:  %[[COORD:.*]] = fir.coordinate_of %[[CONV]], %[[INDEX]] : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
diff --git a/flang/test/Lower/OpenMP/default-clause-byref.f90 b/flang/test/Lower/OpenMP/default-clause-byref.f90
index 7e9011f9c1bd56..6cdff407a97901 100644
--- a/flang/test/Lower/OpenMP/default-clause-byref.f90
+++ b/flang/test/Lower/OpenMP/default-clause-byref.f90
@@ -74,7 +74,7 @@
 !CHECK: %[[Z:.*]] = fir.alloca i32 {bindc_name = "z", uniq_name = "_QFEz"}
 !CHECK: %[[Z_DECL:.*]]:2 = hlfir.declare %[[Z]] {uniq_name = "_QFEz"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: omp.parallel private(
-!CHECK-SAME: @[[X_FIRSTPRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]] : {{.*}}, @[[Y_PRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]] : {{.*}}, @[[W_PRIVATIZER]] %[[W_DECL]]#0 -> %[[PRIVATE_W:.*]] : {{.*}}) {
+!CHECK-SAME: @[[X_FIRSTPRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]], @[[Y_PRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]], @[[W_PRIVATIZER]] %[[W_DECL]]#0 -> %[[PRIVATE_W:.*]] : {{.*}}) {
 !CHECK: %[[PRIVATE_X_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_X]] {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_Y_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_Y]] {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_W_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_W]] {uniq_name = "_QFEw"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
@@ -108,7 +108,7 @@ program default_clause_lowering
     !$omp end parallel
 
 !CHECK: omp.parallel private(
-!CHECK-SAME: @[[X_PRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]] : {{.*}}, @[[Y_PRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]] : {{.*}}) {
+!CHECK-SAME: @[[X_PRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]], @[[Y_PRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]] : {{.*}}) {
 !CHECK: %[[PRIVATE_X_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_X]] {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_Y_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_Y]] {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[TEMP:.*]] = fir.load %[[PRIVATE_Y_DECL]]#0 : !fir.ref<i32>
@@ -121,7 +121,7 @@ program default_clause_lowering
     !$omp end parallel
 
 !CHECK: omp.parallel private(
-!CHECK-SAME: @[[Y_FIRSTPRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]] : {{.*}}, @[[X_FIRSTPRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]] : {{.*}}) {
+!CHECK-SAME: @[[Y_FIRSTPRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]], @[[X_FIRSTPRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]] : {{.*}}) {
 !CHECK: %[[PRIVATE_Y_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_Y]] {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_X_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_X]] {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[TEMP:.*]] = fir.load %[[PRIVATE_Y_DECL]]#0 : !fir.ref<i32>
@@ -134,7 +134,7 @@ program default_clause_lowering
     !$omp end parallel
 
 !CHECK: omp.parallel private(
-!CHECK-SAME: @[[X_PRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]] : {{.*}}, @[[Y_FIRSTPRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]] : {{.*}}, @[[W_FIRSTPRIVATIZER]] %[[W_DECL]]#0 -> %[[PRIVATE_W:.*]] : {{.*}}) {
+!CHECK-SAME: @[[X_PRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]], @[[Y_FIRSTPRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]], @[[W_FIRSTPRIVATIZER]] %[[W_DECL]]#0 -> %[[PRIVATE_W:.*]] : {{.*}}) {
 !CHECK: %[[PRIVATE_X_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_X]] {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_Y_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_Y]] {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_W_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_W]] {uniq_name = "_QFEw"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
@@ -156,7 +156,7 @@ program default_clause_lowering
 
 !CHECK: omp.parallel   {
 !CHECK: omp.parallel   private(
-!CHECK-SAME: @[[X_PRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]] : {{.*}}, @[[Y_PRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]] : {{.*}}) {
+!CHECK-SAME: @[[X_PRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]], @[[Y_PRIVATIZER]] %[[Y_DECL]]#0 -> %[[PRIVATE_Y:.*]] : {{.*}}) {
 !CHECK: %[[PRIVATE_X_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_X]] {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_Y_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_Y]] {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[TEMP:.*]] = fir.load %[[PRIVATE_Y_DECL]]#0 : !fir.ref<i32>
@@ -164,7 +164,7 @@ program default_clause_lowering
 !CHECK: omp.terminator
 !CHECK: }
 !CHECK: omp.parallel private(
-!CHECK-SAME: @[[W_FIRSTPRIVATIZER]] %[[W_DECL]]#0 -> %[[PRIVATE_W:.*]] : {{.*}}, @[[X_FIRSTPRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]] : {{.*}}) {
+!CHECK-SAME: @[[W_FIRSTPRIVATIZER]] %[[W_DECL]]#0 -> %[[PRIVATE_W:.*]], @[[X_FIRSTPRIVATIZER]] %[[X_DECL]]#0 -> %[[PRIVATE_X:.*]] : {{.*}}) {
 !CHECK: %[[PRIVATE_W_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_W]] {uniq_name = "_QFEw"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[PRIVATE_X_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_X]] {uniq_name = "_QFEx"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[TEMP:.*]] = fir.load %[[PRIVATE_X_DECL]]#0 : !fir.ref<i32>
@@ -197,12 +197,12 @@ subroutine nested_default_clause_tests
 !CHECK: %[[Y_DECL:.*]]:2 = hlfir.declare %[[Y]] {uniq_name = "_QFnested_default_clause_testsEy"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK: %[[Z:.*]] = fir.alloca i32 {bindc_name = "z", uniq_name = "_QFnested_default_clause_testsEz"}
 !CHECK: %[[Z_DECL:.*]]:2 = hlfir.declare %[[Z]] {uniq_name = "_QFnested_default_clause_testsEz"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
-!CHECK: omp.parallel private({{.*firstprivate.*}} {{.*}}#0 -> %[[PRIVATE_X:.*]] : {{.*}}, {{.*}} {{.*}}#0 -> %[[PRIVATE_Y:.*]] : {{.*}}, {{.*}} {{.*}}#0 -> %[[PRIVATE_Z:.*]] : {{.*}}, {{.*}} {{.*}}#0 -> %[[PRIVATE_K:.*]] : {{.*}}) {
+!CHECK: omp.parallel private({{.*firstprivate.*}} {{.*}}#0 -> %[[PRIVATE_X:.*]], {{.*}} {{.*}}#0 -> %[[PRIVATE_Y:.*]], {{.*}} {{.*}}#0 -> %[[PRIVATE_Z:.*]], {{.*}} {{.*}}#0 -> %[[PRIVATE_K:.*]] : {{.*}}) {
 !CHECK: %[[PRIVATE_X_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_X...
[truncated]

Copy link
Contributor

@tblah tblah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cleanup!

@skatrak skatrak force-pushed the users/skatrak/block-args-01-iface branch from 4555bc4 to e5a0b3c Compare October 1, 2024 13:29
Base automatically changed from users/skatrak/block-args-01-iface to main October 1, 2024 14:04
…lauses

This patch updates printing and parsing of operations including clauses that
define entry block arguments to the operation's region. This impacts
`in_reduction`, `map`, `private`, `reduction` and `task_reduction`.

The proposed representation to be used by all such clauses is the following:
```
<clause_name>([byref] [@<sym>] %value -> %block_arg [, ...] : <type>[, ...]) {
  ...
}
```

The `byref` tag is only allowed for reduction-like clauses and the `@<sym>` is
required and only allowed for the `private` and reduction-like clauses. The
`map` clause does not accept any of these two.

This change fixes some currently broken op representations, like `omp.teams` or
`omp.sections` reduction:
```
omp.teams reduction([byref] @<sym> -> %value : <type>) {
^bb0(%block_arg : <type>):
  ...
}
```

Additionally, it addresses some redundancy in the representation of the
previously mentioned cases, as well as e.g. `map` in `omp.target`. The problem
is that the block argument name after the arrow is not checked in any way,
which makes some misleading representations legal:
```mlir
omp.target map_entries(%x -> %arg1, %y -> %arg0, %z -> %doesnt_exist : !llvm.ptr, !llvm.ptr, !llvm.ptr) {
^bb0(%arg0 : !llvm.ptr, %arg1 : !llvm.ptr, %arg2 : !llvm.ptr):
  ...
}
```

In that case, `%x` maps to `%arg0`, contrary to what the representation states,
and `%z` maps to `%arg2`. `%doesnt_exist` is not resolved, so it would likely
cause issues if used anywhere inside of the operation's region.

The solution implemented in this patch makes it so that values introduced after
the arrow on the representation of these clauses implicitly define the
corresponding entry block arguments, removing the potential for these
problematic representations. This is what is already implemented for the
`private` and `reduction` clauses of `omp.parallel`.

There are a couple of consequences of this change:
- Entry block argument-defining clauses must come at the end of the operation's
representation and in alphabetical order. This is because they are
printed/parsed as part of the region and a standardized ordering is needed to
reliably match op arguments with their corresponding entry block arguments via
the `BlockArgOpenMPOpInterface`.
- We can no longer define per-clause assembly formats to be reused by all
operations that take these clauses, since they must be passed to a custom
printer including the region and arguments of all other entry block
argument-defining clauses. Code duplication and potential for introducing
issues is minimized by providing the generic `{print,parse}BlockArgRegion`
helpers and associated structures.

MLIR and Flang lowering unit tests are updated due to changes in the order and
formatting of impacted operations.
@skatrak skatrak force-pushed the users/skatrak/block-args-02-asmfmt branch from a757969 to 181f7b6 Compare October 1, 2024 14:46
@skatrak skatrak merged commit cdb3ebf into main Oct 1, 2024
6 of 7 checks passed
@skatrak skatrak deleted the users/skatrak/block-args-02-asmfmt branch October 1, 2024 15:18
Sterling-Augustine pushed a commit to Sterling-Augustine/llvm-project that referenced this pull request Oct 3, 2024
…lauses (llvm#109809)

This patch updates printing and parsing of operations including clauses
that define entry block arguments to the operation's region. This
impacts `in_reduction`, `map`, `private`, `reduction` and
`task_reduction`.

The proposed representation to be used by all such clauses is the
following:
```
<clause_name>([byref] [@<sym>] %value -> %block_arg [, ...] : <type>[, ...]) {
  ...
}
```

The `byref` tag is only allowed for reduction-like clauses and the
`@<sym>` is required and only allowed for the `private` and
reduction-like clauses. The `map` clause does not accept any of these
two.

This change fixes some currently broken op representations, like
`omp.teams` or `omp.sections` reduction:
```
omp.teams reduction([byref] @<sym> -> %value : <type>) {
^bb0(%block_arg : <type>):
  ...
}
```

Additionally, it addresses some redundancy in the representation of the
previously mentioned cases, as well as e.g. `map` in `omp.target`. The
problem is that the block argument name after the arrow is not checked
in any way, which makes some misleading representations legal:
```mlir
omp.target map_entries(%x -> %arg1, %y -> %arg0, %z -> %doesnt_exist : !llvm.ptr, !llvm.ptr, !llvm.ptr) {
^bb0(%arg0 : !llvm.ptr, %arg1 : !llvm.ptr, %arg2 : !llvm.ptr):
  ...
}
```

In that case, `%x` maps to `%arg0`, contrary to what the representation
states, and `%z` maps to `%arg2`. `%doesnt_exist` is not resolved, so it
would likely cause issues if used anywhere inside of the operation's
region.

The solution implemented in this patch makes it so that values
introduced after the arrow on the representation of these clauses
implicitly define the corresponding entry block arguments, removing the
potential for these problematic representations. This is what is already
implemented for the `private` and `reduction` clauses of `omp.parallel`.

There are a couple of consequences of this change:
- Entry block argument-defining clauses must come at the end of the
operation's representation and in alphabetical order. This is because
they are printed/parsed as part of the region and a standardized
ordering is needed to reliably match op arguments with their
corresponding entry block arguments via the `BlockArgOpenMPOpInterface`.
- We can no longer define per-clause assembly formats to be reused by
all operations that take these clauses, since they must be passed to a
custom printer including the region and arguments of all other entry
block argument-defining clauses. Code duplication and potential for
introducing issues is minimized by providing the generic
`{print,parse}BlockArgRegion` helpers and associated structures.

MLIR and Flang lowering unit tests are updated due to changes in the
order and formatting of impacted operations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants