[MLIR][OpenMP] NFC: Uniformize OpenMP ops names #85393
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
skatrak
requested review from
DavidTruby,
ergawy,
jsjodin,
tblah,
kiranchandramohan,
abidh,
raghavendhra,
agozillon,
DominikAdamski,
luporl,
TIFitis,
kparzysz and
bhandarkar-pranav
llvmbot
added
mlir:llvm
mlir
flang
|
@llvm/pr-subscribers-flang-driver @llvm/pr-subscribers-mlir-llvm Author: Sergio Afonso (skatrak) ChangesThis patch proposes the renaming of certain OpenMP dialect operations with the goal of improving readability and following a uniform naming convention for MLIR operations and associated classes. In particular, the following operations are renamed:
Also, the following MLIR operation classes have been renamed:
Patch is 223.23 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/85393.diff 39 Files Affected:
diff --git a/flang/docs/OpenMP-descriptor-management.md b/flang/docs/OpenMP-descriptor-management.md
index 90a20282e05126..368ff3e911fcb2 100644
--- a/flang/docs/OpenMP-descriptor-management.md
+++ b/flang/docs/OpenMP-descriptor-management.md
@@ -49,15 +49,16 @@ the lowering to FIR/HLFIR has been performed an OpenMP dialect specific pass for
mappings, with one extra per pointer member in the descriptor that is supported on top of the original
descriptor map operation. These pointers members are linked to the parent descriptor by adding them to
the member field of the original descriptor map operation, they are then inserted into the relevant map
-owning operation's (`omp.TargetOp`, `omp.DataOp` etc.) map operand list and in cases where the owning operation
-is `IsolatedFromAbove`, it also inserts them as `BlockArgs` to canonicalize the mappings and simplify lowering.
+owning operation's (`omp.TargetOp`, `omp.TargetDataOp` etc.) map operand list and in cases where the owning
+operation is `IsolatedFromAbove`, it also inserts them as `BlockArgs` to canonicalize the mappings and
+simplify lowering.
An example transformation by the `OMPDescriptorMapInfoGenPass`:
```
...
-%12 = omp.map_info var_ptr(%1#1 : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.box<!fir.ptr<!fir.array<?xi32>>>) map_clauses(tofrom) capture(ByRef) bounds(%11) -> !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>> {name = "arg_alloc"}
+%12 = omp.map.info var_ptr(%1#1 : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.box<!fir.ptr<!fir.array<?xi32>>>) map_clauses(tofrom) capture(ByRef) bounds(%11) -> !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>> {name = "arg_alloc"}
...
omp.target map_entries(%12 -> %arg1, %13 -> %arg2 : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.ref<i32>) {
^bb0(%arg1: !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, %arg2: !fir.ref<i32>):
@@ -67,8 +68,8 @@ omp.target map_entries(%12 -> %arg1, %13 -> %arg2 : !fir.ref<!fir.box<!fir.ptr<!
...
%12 = fir.box_offset %1#1 base_addr : (!fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>) -> !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>
-%13 = omp.map_info var_ptr(%1#1 : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.array<?xi32>) var_ptr_ptr(%12 : !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>) map_clauses(tofrom) capture(ByRef) bounds(%11) -> !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>> {name = ""}
-%14 = omp.map_info var_ptr(%1#1 : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.box<!fir.ptr<!fir.array<?xi32>>>) map_clauses(tofrom) capture(ByRef) members(%13 : !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>) -> !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>> {name = "arg_alloc"}
+%13 = omp.map.info var_ptr(%1#1 : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.array<?xi32>) var_ptr_ptr(%12 : !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>) map_clauses(tofrom) capture(ByRef) bounds(%11) -> !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>> {name = ""}
+%14 = omp.map.info var_ptr(%1#1 : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.box<!fir.ptr<!fir.array<?xi32>>>) map_clauses(tofrom) capture(ByRef) members(%13 : !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>) -> !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>> {name = "arg_alloc"}
...
omp.target map_entries(%13 -> %arg1, %14 -> %arg2, %15 -> %arg3 : !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>, !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.ref<i32>) {
^bb0(%arg1: !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>, %arg2: !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, %arg3: !fir.ref<i32>):
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index 25bb4d9cff5d16..f639620e5e7142 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -465,7 +465,7 @@ static void genBodyOfTargetDataOp(
Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
Fortran::lower::pft::Evaluation &eval, bool genNested,
- mlir::omp::DataOp &dataOp,
+ mlir::omp::TargetDataOp &dataOp,
const llvm::SmallVector<mlir::Type> &useDeviceTypes,
const llvm::SmallVector<mlir::Location> &useDeviceLocs,
const llvm::SmallVector<const Fortran::semantics::Symbol *>
@@ -779,8 +779,8 @@ genTaskOp(Fortran::lower::AbstractConverter &converter,
dependOperands, allocateOperands, allocatorOperands);
}
-static mlir::omp::TaskGroupOp
-genTaskGroupOp(Fortran::lower::AbstractConverter &converter,
+static mlir::omp::TaskgroupOp
+genTaskgroupOp(Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
Fortran::lower::pft::Evaluation &eval, bool genNested,
mlir::Location currentLocation,
@@ -790,7 +790,7 @@ genTaskGroupOp(Fortran::lower::AbstractConverter &converter,
cp.processAllocate(allocatorOperands, allocateOperands);
cp.processTODO<Fortran::parser::OmpClause::TaskReduction>(
currentLocation, llvm::omp::Directive::OMPD_taskgroup);
- return genOpWithBody<mlir::omp::TaskGroupOp>(
+ return genOpWithBody<mlir::omp::TaskgroupOp>(
OpWithBodyGenInfo(converter, semaCtx, currentLocation, eval)
.setGenNested(genNested)
.setClauses(&clauseList),
@@ -850,12 +850,12 @@ static void promoteNonCPtrUseDevicePtrArgsToUseDeviceAddr(
}
}
-static mlir::omp::DataOp
-genDataOp(Fortran::lower::AbstractConverter &converter,
- Fortran::semantics::SemanticsContext &semaCtx,
- Fortran::lower::pft::Evaluation &eval, bool genNested,
- mlir::Location currentLocation,
- const Fortran::parser::OmpClauseList &clauseList) {
+static mlir::omp::TargetDataOp
+genTargetDataOp(Fortran::lower::AbstractConverter &converter,
+ Fortran::semantics::SemanticsContext &semaCtx,
+ Fortran::lower::pft::Evaluation &eval, bool genNested,
+ mlir::Location currentLocation,
+ const Fortran::parser::OmpClauseList &clauseList) {
Fortran::lower::StatementContext stmtCtx;
mlir::Value ifClauseOperand, deviceOperand;
llvm::SmallVector<mlir::Value> mapOperands, devicePtrOperands,
@@ -889,7 +889,7 @@ genDataOp(Fortran::lower::AbstractConverter &converter,
cp.processMap(currentLocation, llvm::omp::Directive::OMPD_target_data,
stmtCtx, mapOperands);
- auto dataOp = converter.getFirOpBuilder().create<mlir::omp::DataOp>(
+ auto dataOp = converter.getFirOpBuilder().create<mlir::omp::TargetDataOp>(
currentLocation, ifClauseOperand, deviceOperand, devicePtrOperands,
deviceAddrOperands, mapOperands);
genBodyOfTargetDataOp(converter, semaCtx, eval, genNested, dataOp,
@@ -900,7 +900,7 @@ genDataOp(Fortran::lower::AbstractConverter &converter,
template <typename OpTy>
static OpTy
-genEnterExitUpdateDataOp(Fortran::lower::AbstractConverter &converter,
+genTargetEnterExitUpdateDataOp(Fortran::lower::AbstractConverter &converter,
Fortran::semantics::SemanticsContext &semaCtx,
mlir::Location currentLocation,
const Fortran::parser::OmpClauseList &clauseList) {
@@ -914,15 +914,15 @@ genEnterExitUpdateDataOp(Fortran::lower::AbstractConverter &converter,
Fortran::parser::OmpIfClause::DirectiveNameModifier directiveName;
// GCC 9.3.0 emits a (probably) bogus warning about an unused variable.
[[maybe_unused]] llvm::omp::Directive directive;
- if constexpr (std::is_same_v<OpTy, mlir::omp::EnterDataOp>) {
+ if constexpr (std::is_same_v<OpTy, mlir::omp::TargetEnterDataOp>) {
directiveName =
Fortran::parser::OmpIfClause::DirectiveNameModifier::TargetEnterData;
directive = llvm::omp::Directive::OMPD_target_enter_data;
- } else if constexpr (std::is_same_v<OpTy, mlir::omp::ExitDataOp>) {
+ } else if constexpr (std::is_same_v<OpTy, mlir::omp::TargetExitDataOp>) {
directiveName =
Fortran::parser::OmpIfClause::DirectiveNameModifier::TargetExitData;
directive = llvm::omp::Directive::OMPD_target_exit_data;
- } else if constexpr (std::is_same_v<OpTy, mlir::omp::UpdateDataOp>) {
+ } else if constexpr (std::is_same_v<OpTy, mlir::omp::TargetUpdateOp>) {
directiveName =
Fortran::parser::OmpIfClause::DirectiveNameModifier::TargetUpdate;
directive = llvm::omp::Directive::OMPD_target_update;
@@ -936,7 +936,7 @@ genEnterExitUpdateDataOp(Fortran::lower::AbstractConverter &converter,
cp.processDepend(dependTypeOperands, dependOperands);
cp.processNowait(nowaitAttr);
- if constexpr (std::is_same_v<OpTy, mlir::omp::UpdateDataOp>) {
+ if constexpr (std::is_same_v<OpTy, mlir::omp::TargetUpdateOp>) {
cp.processMotionClauses<Fortran::parser::OmpClause::To>(stmtCtx,
mapOperands);
cp.processMotionClauses<Fortran::parser::OmpClause::From>(stmtCtx,
@@ -1409,19 +1409,19 @@ genOmpSimpleStandalone(Fortran::lower::AbstractConverter &converter,
firOpBuilder.create<mlir::omp::TaskyieldOp>(currentLocation);
break;
case llvm::omp::Directive::OMPD_target_data:
- genDataOp(converter, semaCtx, eval, genNested, currentLocation,
- opClauseList);
+ genTargetDataOp(converter, semaCtx, eval, genNested, currentLocation,
+ opClauseList);
break;
case llvm::omp::Directive::OMPD_target_enter_data:
- genEnterExitUpdateDataOp<mlir::omp::EnterDataOp>(
+ genTargetEnterExitUpdateDataOp<mlir::omp::TargetEnterDataOp>(
converter, semaCtx, currentLocation, opClauseList);
break;
case llvm::omp::Directive::OMPD_target_exit_data:
- genEnterExitUpdateDataOp<mlir::omp::ExitDataOp>(
+ genTargetEnterExitUpdateDataOp<mlir::omp::TargetExitDataOp>(
converter, semaCtx, currentLocation, opClauseList);
break;
case llvm::omp::Directive::OMPD_target_update:
- genEnterExitUpdateDataOp<mlir::omp::UpdateDataOp>(
+ genTargetEnterExitUpdateDataOp<mlir::omp::TargetUpdateOp>(
converter, semaCtx, currentLocation, opClauseList);
break;
case llvm::omp::Directive::OMPD_ordered:
@@ -1907,15 +1907,15 @@ genOMP(Fortran::lower::AbstractConverter &converter,
beginClauseList, directive.v);
break;
case llvm::omp::Directive::OMPD_target_data:
- genDataOp(converter, semaCtx, eval, /*genNested=*/true, currentLocation,
- beginClauseList);
+ genTargetDataOp(converter, semaCtx, eval, /*genNested=*/true,
+ currentLocation, beginClauseList);
break;
case llvm::omp::Directive::OMPD_task:
genTaskOp(converter, semaCtx, eval, /*genNested=*/true, currentLocation,
beginClauseList);
break;
case llvm::omp::Directive::OMPD_taskgroup:
- genTaskGroupOp(converter, semaCtx, eval, /*genNested=*/true,
+ genTaskgroupOp(converter, semaCtx, eval, /*genNested=*/true,
currentLocation, beginClauseList);
break;
case llvm::omp::Directive::OMPD_teams:
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 a1fc614334dbce..98e5d0b76ea848 100644
--- a/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
+++ b/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir
@@ -232,39 +232,39 @@ func.func @_QPomp_target_data() {
%c0 = arith.constant 0 : index
%4 = arith.subi %c1024, %c1 : index
%5 = omp.bounds lower_bound(%c0 : index) upper_bound(%4 : index) extent(%c1024 : index) stride(%c1 : index) start_idx(%c1 : index)
- %6 = omp.map_info var_ptr(%0 : !fir.ref<!fir.array<1024xi32>>, !fir.array<1024xi32>) map_clauses(to) capture(ByRef) bounds(%5) -> !fir.ref<!fir.array<1024xi32>> {name = "a"}
+ %6 = omp.map.info var_ptr(%0 : !fir.ref<!fir.array<1024xi32>>, !fir.array<1024xi32>) map_clauses(to) capture(ByRef) bounds(%5) -> !fir.ref<!fir.array<1024xi32>> {name = "a"}
%c1_3 = arith.constant 1 : index
%c0_4 = arith.constant 0 : index
%7 = arith.subi %c1024_0, %c1_3 : index
%8 = omp.bounds lower_bound(%c0_4 : index) upper_bound(%7 : index) extent(%c1024_0 : index) stride(%c1_3 : index) start_idx(%c1_3 : index)
- %9 = omp.map_info var_ptr(%1 : !fir.ref<!fir.array<1024xi32>>, !fir.array<1024xi32>) map_clauses(to) capture(ByRef) bounds(%8) -> !fir.ref<!fir.array<1024xi32>> {name = "b"}
+ %9 = omp.map.info var_ptr(%1 : !fir.ref<!fir.array<1024xi32>>, !fir.array<1024xi32>) map_clauses(to) capture(ByRef) bounds(%8) -> !fir.ref<!fir.array<1024xi32>> {name = "b"}
%c1_5 = arith.constant 1 : index
%c0_6 = arith.constant 0 : index
%10 = arith.subi %c1024_1, %c1_5 : index
%11 = omp.bounds lower_bound(%c0_6 : index) upper_bound(%10 : index) extent(%c1024_1 : index) stride(%c1_5 : index) start_idx(%c1_5 : index)
- %12 = omp.map_info var_ptr(%2 : !fir.ref<!fir.array<1024xi32>>, !fir.array<1024xi32>) map_clauses(always, exit_release_or_enter_alloc) capture(ByRef) bounds(%11) -> !fir.ref<!fir.array<1024xi32>> {name = "c"}
- omp.target_enter_data map_entries(%6, %9, %12 : !fir.ref<!fir.array<1024xi32>>, !fir.ref<!fir.array<1024xi32>>, !fir.ref<!fir.array<1024xi32>>)
+ %12 = omp.map.info var_ptr(%2 : !fir.ref<!fir.array<1024xi32>>, !fir.array<1024xi32>) map_clauses(always, exit_release_or_enter_alloc) capture(ByRef) bounds(%11) -> !fir.ref<!fir.array<1024xi32>> {name = "c"}
+ omp.target.enterdata map_entries(%6, %9, %12 : !fir.ref<!fir.array<1024xi32>>, !fir.ref<!fir.array<1024xi32>>, !fir.ref<!fir.array<1024xi32>>)
%c1_7 = arith.constant 1 : index
%c0_8 = arith.constant 0 : index
%13 = arith.subi %c1024, %c1_7 : index
%14 = omp.bounds lower_bound(%c0_8 : index) upper_bound(%13 : index) extent(%c1024 : index) stride(%c1_7 : index) start_idx(%c1_7 : index)
- %15 = omp.map_info var_ptr(%0 : !fir.ref<!fir.array<1024xi32>>, !fir.array<1024xi32>) map_clauses(from) capture(ByRef) bounds(%14) -> !fir.ref<!fir.array<1024xi32>> {name = "a"}
+ %15 = omp.map.info var_ptr(%0 : !fir.ref<!fir.array<1024xi32>>, !fir.array<1024xi32>) map_clauses(from) capture(ByRef) bounds(%14) -> !fir.ref<!fir.array<1024xi32>> {name = "a"}
%c1_9 = arith.constant 1 : index
%c0_10 = arith.constant 0 : index
%16 = arith.subi %c1024_0, %c1_9 : index
%17 = omp.bounds lower_bound(%c0_10 : index) upper_bound(%16 : index) extent(%c1024_0 : index) stride(%c1_9 : index) start_idx(%c1_9 : index)
- %18 = omp.map_info var_ptr(%1 : !fir.ref<!fir.array<1024xi32>>, !fir.array<1024xi32>) map_clauses(from) capture(ByRef) bounds(%17) -> !fir.ref<!fir.array<1024xi32>> {name = "b"}
+ %18 = omp.map.info var_ptr(%1 : !fir.ref<!fir.array<1024xi32>>, !fir.array<1024xi32>) map_clauses(from) capture(ByRef) bounds(%17) -> !fir.ref<!fir.array<1024xi32>> {name = "b"}
%c1_11 = arith.constant 1 : index
%c0_12 = arith.constant 0 : index
%19 = arith.subi %c1024_1, %c1_11 : index
%20 = omp.bounds lower_bound(%c0_12 : index) upper_bound(%19 : index) extent(%c1024_1 : index) stride(%c1_11 : index) start_idx(%c1_11 : index)
- %21 = omp.map_info var_ptr(%2 : !fir.ref<!fir.array<1024xi32>>, !fir.array<1024xi32>) map_clauses(exit_release_or_enter_alloc) capture(ByRef) bounds(%20) -> !fir.ref<!fir.array<1024xi32>> {name = "c"}
+ %21 = omp.map.info var_ptr(%2 : !fir.ref<!fir.array<1024xi32>>, !fir.array<1024xi32>) map_clauses(exit_release_or_enter_alloc) capture(ByRef) bounds(%20) -> !fir.ref<!fir.array<1024xi32>> {name = "c"}
%c1_13 = arith.constant 1 : index
%c0_14 = arith.constant 0 : index
%22 = arith.subi %c1024_2, %c1_13 : index
%23 = omp.bounds lower_bound(%c0_14 : index) upper_bound(%22 : index) extent(%c1024_2 : index) stride(%c1_13 : index) start_idx(%c1_13 : index)
- %24 = omp.map_info var_ptr(%3 : !fir.ref<!fir.array<1024xi32>>, !fir.array<1024xi32>) map_clauses(always, delete) capture(ByRef) bounds(%23) -> !fir.ref<!fir.array<1024xi32>> {name = "d"}
- omp.target_exit_data map_entries(%15, %18, %21, %24 : !fir.ref<!fir.array<1024xi32>>, !fir.ref<!fir.array<1024xi32>>, !fir.ref<!fir.array<1024xi32>>, !fir.ref<!fir.array<1024xi32>>)
+ %24 = omp.map.info var_ptr(%3 : !fir.ref<!fir.array<1024xi32>>, !fir.array<1024xi32>) map_clauses(always, delete) capture(ByRef) bounds(%23) -> !fir.ref<!fir.array<1024xi32>> {name = "d"}
+ omp.target.exitdata map_entries(%15, %18, %21, %24 : !fir.ref<!fir.array<1024xi32>>, !fir.ref<!fir.array<1024xi32>>, !fir.ref<!fir.array<1024xi32>>, !fir.ref<!fir.array<1024xi32>>)
return
}
@@ -285,39 +285,39 @@ func.func @_QPomp_target_data() {
// CHECK: %13 = llvm.mlir.constant(0 : index) : i64
// CHECK: %14 = llvm.mlir.constant(1023 : index) : i64
// CHECK: %15 = omp.bounds lower_bound(%13 : i64) upper_bound(%14 : i64) extent(%0 : i64) stride(%12 : i64) start_idx(%12 : i64)
- // CHECK: %16 = omp.map_info var_ptr(%[[VAL_1]] : !llvm.ptr, !llvm.array<1024 x i32>) map_clauses(to) capture(ByRef) bounds(%15) -> !llvm.ptr {name = "a"}
+ // CHECK: %16 = omp.map.info var_ptr(%[[VAL_1]] : !llvm.ptr, !llvm.array<1024 x i32>) map_clauses(to) capture(ByRef) bounds(%15) -> !llvm.ptr {name = "a"}
// CHECK: %17 = llvm.mlir.constant(1 : index) : i64
// CHECK: %18 = llvm.mlir.constant(0 : index) : i64
// CHECK: %19 = llvm.mlir.constant(1023 : index) : i64
// CHECK: %20 = omp.bounds lower_bound(%18 : i64) upper_bound(%19 : i64) extent(%3 : i64) stride(%17 : i64) start_idx(%17 : i64)
- // CHECK: %21 = omp.map_info var_ptr(%[[VAL_3]] : !llvm.ptr, !llvm.array<1024 x i32>) map_clauses(to) capture(ByRef) bounds(%20) -> !llvm.ptr {name = "b"}
+ // CHECK: %21 = omp.map.info var_ptr(%[[VAL_3]] : !llvm.ptr, !llvm.array<1024 x i32>) map_clauses(to) capture(ByRef) bounds(%20) -> !llvm.ptr {name = "b"}
// CHECK: %22 = llvm.mlir.constant(1 : index) : i64
// CHECK: %23 = llvm.mlir.constant(0 : index) : i64
// CHECK: %24 = llvm.mlir.constant(1023 : index) : i64
// CHECK: %25 = omp.bounds lower_bound(%23 : i64) upper_bound(%24 : i64) extent(%6 : i64) stride(%22 : i64) start_idx(%22 : i64)
- // CHECK: %26 = omp.map_info var_ptr(%[[VAL_5]] : !llvm.ptr, !llvm.array<1024 x i32>) map_clauses(always, exit_release_or_enter_alloc) capture(ByRef) bounds(%25) -> !llvm.ptr {name = "c"}
- // CHECK: omp.target_enter_data map_entries(%16, %21, %26 : !llvm.ptr, !llvm.ptr, !llvm.ptr)
+ // CHECK: %26 = omp.map.info var_ptr(%[[VAL_5]] : !llvm.ptr, !llvm.array<1024 x i32>) map_clauses(always, exit_release_or_enter_alloc) capture(ByRef) bounds(%25) -> !llvm.ptr {name = "c"}
+ // CHECK: omp.target.enterdata map_entries(%16, %21, %26 : !llvm.ptr, !llvm.ptr, !llvm.ptr)
// CHECK: %27 = llvm.mlir.constant(1 : index) : i64
// CHECK: %28 = llvm.mlir.constant(0 : index) : i64
// CHECK: %29 = llvm.mlir.constant(1023 : index) : i64
// CHECK: %30 = omp.bounds lower_bound(%28 : i64) upper_bound(%29 : i64) extent(%0 : i64) stride(%27 : i64) start_idx(%27 : i64)
- // CHECK: %31 = omp.map_info var_ptr(%[[VAL_1]] : !llvm.ptr, !llvm.array<1024 x i32>) map_clauses(from) capture(ByRef) bounds(%30) -> !llvm.ptr {name = "a"}
+ // CHECK: %31 = omp.map.info var_ptr(%[[VAL_1]] : !llvm.ptr, !llvm.array<1024 x i32>) map_clauses(from) capture(ByRef) bounds(%30) -> !llvm.ptr {name = "a"}
// CHECK: %32 = llvm.mlir.constant(1 : index) : i64
// CHECK: %33 = llvm.mlir.constant(0 : index) : i64
// CHECK: %34 = llvm.mlir.constant(1023 : index) : i64
// CHECK: %35 = omp.bounds lower_bound(%33 : i64) upper_bound(%34 : i64) extent(%3 : i64) stride(%32 : i64) start_idx(%32 : i64)
- // CHECK: %36 = omp.map_info var_ptr(%[[VAL_3]] : !llvm.ptr, !llvm.array<1024 x i32>) map_clauses(from) capture(ByRef) bounds(%35) -> !llvm.ptr {name = "b"}
+ // CHECK: %36 = omp.map.info var_ptr(%[[VAL_3]] : !llvm.ptr, !llvm.array<1024 x i32>) map_clauses(from) capture(ByRef) bounds(%35) -> !llvm.ptr {name = "b"}
// CHECK: %37 = llvm.mlir.constant(1 : index) : i64
// CHECK: %38 = llvm.mlir.constant(0 : index) : i64
// CHECK: %39 = llvm.mlir.constant(1023 : index) : i64
// CHECK: %40 = omp.bounds lower_bound(%38 : i64) upper_bound(%39 : i64) extent(%6 : i64) stride(%37 : i64) start_idx(%37 : i64)
- // CHECK: %41 = omp.map_info var_ptr(%[[VAL_5]] : !llvm.ptr, !llvm.array<1024 x i32>) map_clauses(exit_release_or_enter_alloc) capture(ByRef) bounds(%40) -> !llvm.ptr {name = "c"}
+ // CHECK: %41 = omp.map.info var_ptr(...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I like the second part. The way I see the names in the first part is that the period separates higher-level concepts, while underscores are emulating spaces in names. In that sense the original names look good to me, except the "cancellationpoint" which should be "cancellation_point", if we're sticking to the construct names from the spec. Edit: I looked at the SCF dialect to see what convention it uses, and I saw an op named "scf.forall.in_parallel", hence my view of the convention being "<name>.<name>...", where a space in <name> will be _. |
|
Thank you Krzysztof for sharing your thoughts. I don't personally have an issue either way with regards to using dots or underscores for separating words in MLIR ops. The issue at the moment is that we use both, so it'd be nice to make it uniform. If we prefer underscores, then rather than changing the operation names that I initially proposed, in my opinion we should change these instead:
|
The first one doesn't exist in the spec, the second one is "declare reduction", so for consistency we should use the names with "declare_xyz".
The "read", "write", etc. are clauses, so "atomic read" is not a name. I suggest keeping the . as a separator here. |
|
@skatrak I welcome the change. But since it is an externally visible change, please make a quick RFC in MLIR discourse. You may go ahead if there are no concerns posted. |
Why not |
"target enter data" and "target exit data" are construct names in the spec. |
|
Thank you all for your comments. Here's an RFC I just created for this: https://discourse.llvm.org/t/rfc-uniformize-openmp-dialect-operation-names/77715. I updated the proposal from what it's currently done in this patch, hopefully addressing most concerns shared so far. |
kiranchandramohan
approved these changes
Mar 19, 2024
This patch proposes the renaming of certain OpenMP dialect operations with the goal of improving readability and following a uniform naming convention for MLIR operations and associated classes. In particular, the following operations are renamed: - `omp.map_info` -> `omp.map.info` - `omp.target_update_data` -> `omp.target_update` - `omp.ordered_region` -> `omp.ordered.region` - `omp.cancellationpoint` -> `omp.cancellation_point` - `omp.bounds` -> `omp.map.bounds` - `omp.reduction.declare` -> `omp.declare_reduction` Also, the following MLIR operation classes have been renamed: - `omp::TaskLoopOp` -> `omp::TaskloopOp` - `omp::TaskGroupOp` -> `omp::TaskgroupOp` - `omp::DataBoundsOp` -> `omp::MapBoundsOp` - `omp::DataOp` -> `omp::TargetDataOp` - `omp::EnterDataOp` -> `omp::TargetEnterDataOp` - `omp::ExitDataOp` -> `omp::TargetExitDataOp` - `omp::UpdateDataOp` -> `omp::TargetUpdateOp` - `omp::ReductionDeclareOp` -> `omp::DeclareReductionOp` - `omp::WsLoopOp` -> `omp::WsloopOp`
skatrak
added a commit
to skatrak/llvm-project
that referenced
this pull request
Mar 20, 2024
This patch proposes the renaming of certain OpenMP dialect operations with the goal of improving readability and following a uniform naming convention for MLIR operations and associated classes. In particular, the following operations are renamed: - `omp.map_info` -> `omp.map.info` - `omp.target_update_data` -> `omp.target_update` - `omp.ordered_region` -> `omp.ordered.region` - `omp.cancellationpoint` -> `omp.cancellation_point` - `omp.bounds` -> `omp.map.bounds` - `omp.reduction.declare` -> `omp.declare_reduction` Also, the following MLIR operation classes have been renamed: - `omp::TaskLoopOp` -> `omp::TaskloopOp` - `omp::TaskGroupOp` -> `omp::TaskgroupOp` - `omp::DataBoundsOp` -> `omp::MapBoundsOp` - `omp::DataOp` -> `omp::TargetDataOp` - `omp::EnterDataOp` -> `omp::TargetEnterDataOp` - `omp::ExitDataOp` -> `omp::TargetExitDataOp` - `omp::UpdateDataOp` -> `omp::TargetUpdateOp` - `omp::ReductionDeclareOp` -> `omp::DeclareReductionOp` - `omp::WsLoopOp` -> `omp::WsloopOp`
skatrak
added a commit
to ROCm/llvm-project
that referenced
this pull request
Mar 20, 2024
* [MLIR][OpenMP] NFC: Uniformize OpenMP ops names (llvm#85393) This patch proposes the renaming of certain OpenMP dialect operations with the goal of improving readability and following a uniform naming convention for MLIR operations and associated classes. In particular, the following operations are renamed: - `omp.map_info` -> `omp.map.info` - `omp.target_update_data` -> `omp.target_update` - `omp.ordered_region` -> `omp.ordered.region` - `omp.cancellationpoint` -> `omp.cancellation_point` - `omp.bounds` -> `omp.map.bounds` - `omp.reduction.declare` -> `omp.declare_reduction` Also, the following MLIR operation classes have been renamed: - `omp::TaskLoopOp` -> `omp::TaskloopOp` - `omp::TaskGroupOp` -> `omp::TaskgroupOp` - `omp::DataBoundsOp` -> `omp::MapBoundsOp` - `omp::DataOp` -> `omp::TargetDataOp` - `omp::EnterDataOp` -> `omp::TargetEnterDataOp` - `omp::ExitDataOp` -> `omp::TargetExitDataOp` - `omp::UpdateDataOp` -> `omp::TargetUpdateOp` - `omp::ReductionDeclareOp` -> `omp::DeclareReductionOp` - `omp::WsLoopOp` -> `omp::WsloopOp`
chencha3
pushed a commit
to chencha3/llvm-project
that referenced
this pull request
Mar 23, 2024
This patch proposes the renaming of certain OpenMP dialect operations with the goal of improving readability and following a uniform naming convention for MLIR operations and associated classes. In particular, the following operations are renamed: - `omp.map_info` -> `omp.map.info` - `omp.target_update_data` -> `omp.target_update` - `omp.ordered_region` -> `omp.ordered.region` - `omp.cancellationpoint` -> `omp.cancellation_point` - `omp.bounds` -> `omp.map.bounds` - `omp.reduction.declare` -> `omp.declare_reduction` Also, the following MLIR operation classes have been renamed: - `omp::TaskLoopOp` -> `omp::TaskloopOp` - `omp::TaskGroupOp` -> `omp::TaskgroupOp` - `omp::DataBoundsOp` -> `omp::MapBoundsOp` - `omp::DataOp` -> `omp::TargetDataOp` - `omp::EnterDataOp` -> `omp::TargetEnterDataOp` - `omp::ExitDataOp` -> `omp::TargetExitDataOp` - `omp::UpdateDataOp` -> `omp::TargetUpdateOp` - `omp::ReductionDeclareOp` -> `omp::DeclareReductionOp` - `omp::WsLoopOp` -> `omp::WsloopOp`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch proposes the renaming of certain OpenMP dialect operations with the
goal of improving readability and following a uniform naming convention for
MLIR operations and associated classes. In particular, the following operations
are renamed:
omp.map_info->omp.map.infoomp.target_update_data->omp.target_updateomp.ordered_region->omp.ordered.regionomp.cancellationpoint->omp.cancellation_pointomp.bounds->omp.map.boundsomp.reduction.declare->omp.declare_reductionAlso, the following MLIR operation classes have been renamed:
omp::TaskLoopOp->omp::TaskloopOpomp::TaskGroupOp->omp::TaskgroupOpomp::DataBoundsOp->omp::MapBoundsOpomp::DataOp->omp::TargetDataOpomp::EnterDataOp->omp::TargetEnterDataOpomp::ExitDataOp->omp::TargetExitDataOpomp::UpdateDataOp->omp::TargetUpdateOpomp::ReductionDeclareOp->omp::DeclareReductionOpomp::WsLoopOp->omp::WsloopOp