[OpenMP][MLIR] Add new arguments to map_info to help support record type maps #82851
Conversation
Created using spr 1.3.4
|
@llvm/pr-subscribers-mlir-openmp @llvm/pr-subscribers-flang-openmp Author: None (agozillon) ChangesThis PR adds two new fields to omp.map_info, one BoolAttr and one I64ArrayAttr. The BoolAttr is named partial_map, and is a flag that indicates if the record type captured by The I64ArrayAttr named members_index is intended to track the placement of each member Full diff: https://github.com/llvm/llvm-project/pull/82851.diff 3 Files Affected:
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
index f907e21e9b4de2..98013dff032cc8 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
@@ -1298,10 +1298,12 @@ def MapInfoOp : OpenMP_Op<"map_info", [AttrSizedOperandSegments]> {
TypeAttr:$var_type,
Optional<OpenMP_PointerLikeType>:$var_ptr_ptr,
Variadic<OpenMP_PointerLikeType>:$members,
+ OptionalAttr<I64ArrayAttr>:$members_index,
Variadic<DataBoundsType>:$bounds, /* rank-0 to rank-{n-1} */
OptionalAttr<UI64Attr>:$map_type,
OptionalAttr<VariableCaptureKindAttr>:$map_capture_type,
- OptionalAttr<StrAttr>:$name);
+ OptionalAttr<StrAttr>:$name,
+ DefaultValuedAttr<BoolAttr, "false">:$partial_map);
let results = (outs OpenMP_PointerLikeType:$omp_ptr);
let description = [{
@@ -1337,10 +1339,14 @@ def MapInfoOp : OpenMP_Op<"map_info", [AttrSizedOperandSegments]> {
- `var_type`: The type of the variable to copy.
- `var_ptr_ptr`: Used when the variable copied is a member of a class, structure
or derived type and refers to the originating struct.
- - `members`: Used to indicate mapped child members for the current MapInfoOp,
+ - `members`: Used to indicate mapped child members for the current MapInfoOp,
represented as other MapInfoOp's, utilised in cases where a parent structure
type and members of the structure type are being mapped at the same time.
- For example: map(to: parent, parent->member, parent->member2[:10])
+ For example: map(to: parent, parent->member, parent->member2[:10])
+ - `members_index`: Used to indicate the ordering of members within the containing
+ parent (generally a record type such as a structure, class or derived type),
+ e.g. struct {int x, float y, double z}, x would be 0, y would be 1, and z
+ would be 2. This aids the mapping
- `bounds`: Used when copying slices of array's, pointers or pointer members of
objects (e.g. derived types or classes), indicates the bounds to be copied
of the variable. When it's an array slice it is in rank order where rank 0
@@ -1351,6 +1357,8 @@ def MapInfoOp : OpenMP_Op<"map_info", [AttrSizedOperandSegments]> {
- 'map_capture_type': Capture type for the variable e.g. this, byref, byvalue, byvla
this can affect how the variable is lowered.
- `name`: Holds the name of variable as specified in user clause (including bounds).
+ - `partial_map`: The record type being mapped will not be mapped in its entirety,
+ it may be used however, in a mapping to bind it's mapped components together.
}];
let assemblyFormat = [{
@@ -1359,7 +1367,7 @@ def MapInfoOp : OpenMP_Op<"map_info", [AttrSizedOperandSegments]> {
`var_ptr_ptr` `(` $var_ptr_ptr `:` type($var_ptr_ptr) `)`
| `map_clauses` `(` custom<MapClause>($map_type) `)`
| `capture` `(` custom<CaptureType>($map_capture_type) `)`
- | `members` `(` $members `:` type($members) `)`
+ | `members` `(` $members `:` type($members) `:` $members_index `)`
| `bounds` `(` $bounds `)`
) `->` type($omp_ptr) attr-dict
}];
diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
index c2b471ab96183f..771ff40736af8b 100644
--- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -915,6 +915,9 @@ static ParseResult parseMapClause(OpAsmParser &parser, IntegerAttr &mapType) {
if (mapTypeMod == "delete")
mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_DELETE;
+ if (mapTypeMod == "ptr_and_obj")
+ mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_PTR_AND_OBJ;
+
return success();
};
@@ -951,6 +954,10 @@ static void printMapClause(OpAsmPrinter &p, Operation *op,
if (mapTypeToBitFlag(mapTypeBits,
llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_PRESENT))
mapTypeStrs.push_back("present");
+ if (mapTypeToBitFlag(
+ mapTypeBits,
+ llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_PTR_AND_OBJ))
+ mapTypeStrs.push_back("ptr_and_obj");
// special handling of to/from/tofrom/delete and release/alloc, release +
// alloc are the abscense of one of the other flags, whereas tofrom requires
diff --git a/mlir/test/Dialect/OpenMP/ops.mlir b/mlir/test/Dialect/OpenMP/ops.mlir
index 211ff0ff9272ec..981923cbdaef4a 100644
--- a/mlir/test/Dialect/OpenMP/ops.mlir
+++ b/mlir/test/Dialect/OpenMP/ops.mlir
@@ -2063,8 +2063,6 @@ func.func @omp_requires_multiple() -> ()
return
}
-// -----
-
// CHECK-LABEL: @opaque_pointers_atomic_rwu
// CHECK-SAME: (%[[v:.*]]: !llvm.ptr, %[[x:.*]]: !llvm.ptr)
func.func @opaque_pointers_atomic_rwu(%v: !llvm.ptr, %x: !llvm.ptr) {
@@ -2171,10 +2169,10 @@ func.func @omp_target_update_data (%if_cond : i1, %device : si32, %map1: memref<
// CHECK-LABEL: omp_targets_is_allocatable
// CHECK-SAME: (%[[ARG0:.*]]: !llvm.ptr, %[[ARG1:.*]]: !llvm.ptr)
func.func @omp_targets_is_allocatable(%arg0: !llvm.ptr, %arg1: !llvm.ptr) -> () {
- // CHECK: %[[MAP0:.*]] = omp.map_info var_ptr(%[[ARG0]] : !llvm.ptr, i32) map_clauses(tofrom) capture(ByRef) -> !llvm.ptr {name = ""}
- %mapv1 = omp.map_info var_ptr(%arg0 : !llvm.ptr, i32) map_clauses(tofrom) capture(ByRef) -> !llvm.ptr {name = ""}
- // CHECK: %[[MAP1:.*]] = omp.map_info var_ptr(%[[ARG1]] : !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8)>) map_clauses(tofrom) capture(ByRef) members(%[[MAP0]] : !llvm.ptr) -> !llvm.ptr {name = ""}
- %mapv2 = omp.map_info var_ptr(%arg1 : !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8)>) map_clauses(tofrom) capture(ByRef) members(%mapv1 : !llvm.ptr) -> !llvm.ptr {name = ""}
+ // CHECK: %[[MAP0:.*]] = omp.map_info var_ptr(%[[ARG0]] : !llvm.ptr, i32) map_clauses(ptr_and_obj, tofrom) capture(ByRef) -> !llvm.ptr {name = ""}
+ %mapv1 = omp.map_info var_ptr(%arg0 : !llvm.ptr, i32) map_clauses(ptr_and_obj, tofrom) capture(ByRef) -> !llvm.ptr {name = ""}
+ // CHECK: %[[MAP1:.*]] = omp.map_info var_ptr(%[[ARG1]] : !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8)>) map_clauses(tofrom) capture(ByRef) members(%[[MAP0]] : !llvm.ptr : [0]) -> !llvm.ptr {name = ""}
+ %mapv2 = omp.map_info var_ptr(%arg1 : !llvm.ptr, !llvm.struct<(ptr, i64, i32, i8, i8, i8, i8)>) map_clauses(tofrom) capture(ByRef) members(%mapv1 : !llvm.ptr : [0]) -> !llvm.ptr {name = ""}
// CHECK: omp.target map_entries(%[[MAP0]] -> {{.*}}, %[[MAP1]] -> {{.*}} : !llvm.ptr, !llvm.ptr)
omp.target map_entries(%mapv1 -> %arg2, %mapv2 -> %arg3 : !llvm.ptr, !llvm.ptr) {
^bb0(%arg2: !llvm.ptr, %arg3 : !llvm.ptr):
@@ -2229,6 +2227,25 @@ func.func @omp_target_enter_update_exit_data_depend(%a: memref<?xi32>, %b: memre
}
// CHECK: omp.target_exit_data map_entries([[MAP2]] : memref<?xi32>) depend(taskdependin -> [[ARG2]] : memref<?xi32>)
omp.target_exit_data map_entries(%map_c : memref<?xi32>) depend(taskdependin -> %c : memref<?xi32>)
+
+ return
+}
+
+// CHECK-LABEL: omp_map_with_members
+// CHECK-SAME: (%[[ARG0:.*]]: !llvm.ptr, %[[ARG1:.*]]: !llvm.ptr, %[[ARG2:.*]]: !llvm.ptr)
+func.func @omp_map_with_members(%arg0: !llvm.ptr, %arg1: !llvm.ptr, %arg2: !llvm.ptr) -> () {
+ // CHECK: %[[MAP0:.*]] = omp.map_info var_ptr(%[[ARG0]] : !llvm.ptr, i32) map_clauses(to) capture(ByRef) -> !llvm.ptr {name = ""}
+ %mapv1 = omp.map_info var_ptr(%arg0 : !llvm.ptr, i32) map_clauses(to) capture(ByRef) -> !llvm.ptr {name = ""}
+
+ // CHECK: %[[MAP1:.*]] = omp.map_info var_ptr(%[[ARG1]] : !llvm.ptr, f32) map_clauses(to) capture(ByRef) -> !llvm.ptr {name = ""}
+ %mapv2 = omp.map_info var_ptr(%arg1 : !llvm.ptr, f32) map_clauses(to) capture(ByRef) -> !llvm.ptr {name = ""}
+
+ // CHECK: %[[MAP2:.*]] = omp.map_info var_ptr(%[[ARG2]] : !llvm.ptr, !llvm.struct<(i32, f32)>) map_clauses(to) capture(ByRef) members(%[[MAP0]], %[[MAP1]] : !llvm.ptr, !llvm.ptr : [0, 1]) -> !llvm.ptr {name = "", partial_map = true}
+ %mapv3 = omp.map_info var_ptr(%arg2 : !llvm.ptr, !llvm.struct<(i32, f32)>) map_clauses(to) capture(ByRef) members(%mapv1, %mapv2 : !llvm.ptr, !llvm.ptr : [0, 1]) -> !llvm.ptr {name = "", partial_map = true}
+
+ // CHECK: omp.target_enter_data map_entries(%[[MAP0]], %[[MAP1]], %[[MAP2]] : !llvm.ptr, !llvm.ptr, !llvm.ptr)
+ omp.target_enter_data map_entries(%mapv1, %mapv2, %mapv3 : !llvm.ptr, !llvm.ptr, !llvm.ptr){}
+
return
}
|
agozillon
requested review from
ergawy,
clementval,
skatrak,
jsjodin,
kiranchandramohan,
raghavendhra,
TIFitis,
kiranktp and
kparzysz
|
Small ping for attention on this PR please, to start nudging it towards the finish line if there is one in its future. I am sorry ahead of time for stealing some of a reviewers precious time. |
|
Is the |
Connected to the overall PR, I should alter the description to change that. Or perhaps simplify the stack by moving the changes into a seperate PR (my original thought was that it would add more dependencies to the PR stack, but I'm not necessarily against that if it makes things easier to review). Do let me know which you'd prefer though! The main goal is to move the setting of the |
|
Yeah, I didn't realize this was a part of a series. :) |
Sorry, I should have made it clearer in the title! I was a little worried about title length as I broke the windows clonability of the llvm-project with a combination of bad commit names and SPR with the previous PR stack, so I avoided adding any index prefix like PR 1/4 etc. I think the morale of the story is that I really need to work a lot on my PR/Commit titles moving forward :-) The full series are the following: #82853 : the flang level changes required, also the top-level PR that should pass CI if all is well in the stack |
|
I'm not sure if member placement alone is sufficient. As I commented in the other PR, for |
It will be extended in a subsequent PR to have N-D indexing, rather than just the 1-D that's currently here, it's only intended to handle the example cases in #82850 for the moment. Which effectively amounts to only being able to map the first layer of a derived type, so no nested derived type member maps. Adding support for derived type member mapping is going to be an iterative process, with support added incrementally, as otherwise the PR would be a little extreme for any reviewer (as well as the fact I am building on the support piece by piece, as has been the case with all of the map work). Current plan is to add gradual map support for the following cases related to derived types:
|
|
So, incremental increase of support if possible and get feedback as early as possible on direction is the aim :-) But if we wanted to avoid that and have it all in a single set of PRs that'd be possible as well, but having this up for an early review is still ideal to see what people feel about the approach. The idea for the member index, at least currently will be to move it to a multi-dimensional index (via DenseIntElementsAttr) as an example: In the above case array%nest would be index: (3) and array%nest%z3 would be index: (3, 2), if we replaced z3 with another layer through a derived type and mapped a member of it, it would be an index of (3, 2, ...) and so on. I am hoping that will be reasonable enough to support the case of map(tofrom: x%y%z, x%y) and from a little IR testing it seems it should be, however, I am still working on the initial implementation for this use-case, so I could be wrong. The main thing I am worried about and a little unsure on currently is if the layout of the derived type will always be segmentable like this. But there could be other things wrong with modeling the placement of members like this that I am missing (or a better method of doing so)! So if there is I would love for some feedback from anyone reading. |
Saying the above, I do have an initial implementation (quicker than I anticipated) that improves on this PR to handle this type of mapping and I do hope to open a PR on it in the coming weeks (provided I don't run into any critical failures along the way), it primarily alters the member index generation, member index storage attribute (DenseIntElementsAttr), and the algorithm that works out which member map is first and last of the mapped members. I primarily need to test and verify it further and make it presentable by tidying it a little and adding the usual array of tests (the most time consuming component) before it's ready for opening. Depending on how far along in the review process this PR is at the time it's ready, I could either update this PR stack with it, wait until this stack has landed to create the PR stack for the next iteration, or add onto the top of this stack with the next one, if any reviewer has any preferences, requests or thoughts on this I would love to hear it. I mainly would like to make the review process as easy as possible for you all as I am aware the change set is fairly broad. And to that affect I'll try and seperate out some of the self-reliant but quality of life changes from this stack into seperate PRs if they can be over the next few days, mainly the MAP_AND_PTR_OBJ changes and the bounds refactoring and movement of the createAlteredByCaptureMap function invocation. |
…ype maps This PR adds two new fields to omp.map_info, one BoolAttr and one I64ArrayAttr. The BoolAttr is named partial_map, and is a flag that indicates if the record type captured by the map_info operation is a partial map, or if it is mapped in its entirety, this currently helps the later lowering determine the type of map entries that need to be generated. The I64ArrayAttr named members_index is intended to track the placement of each member map_info operations (and by extension mapped member variable) placement in the parent record type. This may need to be extended to an N-D array for nested member mapping. Pull Request: llvm#82851
…ype maps This PR adds two new fields to omp.map_info, one BoolAttr and one I64ArrayAttr. The BoolAttr is named partial_map, and is a flag that indicates if the record type captured by the map_info operation is a partial map, or if it is mapped in its entirety, this currently helps the later lowering determine the type of map entries that need to be generated. The I64ArrayAttr named members_index is intended to track the placement of each member map_info operations (and by extension mapped member variable) placement in the parent record type. This may need to be extended to an N-D array for nested member mapping. Pull Request: llvm#82851
Created using spr 1.3.4
…ype maps This PR adds two new fields to omp.map_info, one BoolAttr and one I64ArrayAttr. The BoolAttr is named partial_map, and is a flag that indicates if the record type captured by the map_info operation is a partial map, or if it is mapped in its entirety, this currently helps the later lowering determine the type of map entries that need to be generated. The I64ArrayAttr named members_index is intended to track the placement of each member map_info operations (and by extension mapped member variable) placement in the parent record type. This may need to be extended to an N-D array for nested member mapping. Pull Request: llvm#82851
|
|
||
| static void printMembersIndex(OpAsmPrinter &p, MapInfoOp op, | ||
| DenseIntElementsAttr membersIdx) { | ||
| assert(membersIdx.getShapedType().getShape().size() <= 2); |
There was a problem hiding this comment.
Nit: Maybe store a local reference to membersIdx.getShapedType().getShape() and reuse it to reduce the verbosity of the code a bit.
There was a problem hiding this comment.
Using names for the local references might help also to make the code clearer.
i * membersIdx.getShapedType().getShape()[1] can be hoisted out of the inner loop also.
|
|
||
| static void printMembersIndex(OpAsmPrinter &p, MapInfoOp op, | ||
| DenseIntElementsAttr membersIdx) { | ||
| assert(membersIdx.getShapedType().getShape().size() <= 2); |
There was a problem hiding this comment.
Using names for the local references might help also to make the code clearer.
i * membersIdx.getShapedType().getShape()[1] can be hoisted out of the inner loop also.
Created using spr 1.3.4
…ype maps This PR adds two new fields to omp.map_info, one BoolAttr and one I64ArrayAttr. The BoolAttr is named partial_map, and is a flag that indicates if the record type captured by the map_info operation is a partial map, or if it is mapped in its entirety, this currently helps the later lowering determine the type of map entries that need to be generated. The I64ArrayAttr named members_index is intended to track the placement of each member map_info operations (and by extension mapped member variable) placement in the parent record type. This may need to be extended to an N-D array for nested member mapping. Pull Request: llvm#82851
Created using spr 1.3.4
agozillon
merged commit ff1f4bf
into
users/agozillon/main.openmpmlir-add-new-arguments-to-map_info-to-help-support-record-type-maps
agozillon
deleted the
users/agozillon/openmpmlir-add-new-arguments-to-map_info-to-help-support-record-type-maps
branch
…ype maps This PR adds two new fields to omp.map_info, one BoolAttr and one I64ArrayAttr. The BoolAttr is named partial_map, and is a flag that indicates if the record type captured by the map_info operation is a partial map, or if it is mapped in its entirety, this currently helps the later lowering determine the type of map entries that need to be generated. The I64ArrayAttr named members_index is intended to track the placement of each member map_info operations (and by extension mapped member variable) placement in the parent record type. This may need to be extended to an N-D array for nested member mapping. Pull Request: #82851
|
I made a bit of a mistake not using SPR to land this PR series, so i've ended up having to directly commit the PR, the associated commit for this PR can be found here: 50df0ff |
…ype maps This PR adds two new fields to omp.map_info, one BoolAttr and one I64ArrayAttr. The BoolAttr is named partial_map, and is a flag that indicates if the record type captured by the map_info operation is a partial map, or if it is mapped in its entirety, this currently helps the later lowering determine the type of map entries that need to be generated. The I64ArrayAttr named members_index is intended to track the placement of each member map_info operations (and by extension mapped member variable) placement in the parent record type. This may need to be extended to an N-D array for nested member mapping. Pull Request: llvm#82851
…ype maps This PR adds two new fields to omp.map_info, one BoolAttr and one I64ArrayAttr. The BoolAttr is named partial_map, and is a flag that indicates if the record type captured by the map_info operation is a partial map, or if it is mapped in its entirety, this currently helps the later lowering determine the type of map entries that need to be generated. The I64ArrayAttr named members_index is intended to track the placement of each member map_info operations (and by extension mapped member variable) placement in the parent record type. This may need to be extended to an N-D array for nested member mapping. Pull Request: llvm#82851
This PR adds two new fields to omp.map_info, one BoolAttr and one DenseIntElementsAttr.
The BoolAttr is named partial_map, and is a flag that indicates if the record type captured by
the map_info operation is a partial map, or if it is mapped in its entirety, this currently helps
the later lowering determine the type of map entries that need to be generated.
The DenseIntElementsAttr named members_index is intended to track the placement of each
member map_info operations (and by extension mapped member variable) placement in the
parent record type.