[Flang][MLIR] Fix to mapping adding additional map pointer component for Descriptor mappings, helps to fix use_device_addr/ptr with these types #122
Conversation
…for Descriptor mappings, helps to fix use_device_addr/ptr with these types This PR basically adds an additional pointer binding for the descriptor base address. which we attach the user data to, it also makes sure that the mapping for the descriptor is always at a minimum "to" where possible, as regardless of whether or not we wish to map the user data the descriptor wrapping the data must be mapped across as the kernel will still make an attempt to access descriptor information to perform various tasks such as indexing into the user data. This may eventually be restricted a little further in the future to only allow descriptors to have a to mapping in cases where it's legal to have a to mapping (i.e. not legal on an exit directive).
| op.getMapType().value()), | ||
| builder.getIntegerAttr( | ||
| builder.getIntegerType(64, false), | ||
| getDescriptorMapType(op.getMapType().value(), target)), |
There was a problem hiding this comment.
Do we know for sure the map_type argument is defined by this point or would it make sense to do something like this instead?
| getDescriptorMapType(op.getMapType().value(), target)), | |
| getDescriptorMapType(op.getMapType().value_or(llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_NONE), target)), |
There was a problem hiding this comment.
Actually, maybe we could just make the map_type argument required but default-valued to 0 in MLIR instead.
There was a problem hiding this comment.
The map type should never be unset by Flang, which is the only frontend that will use this pass!
| static_cast< | ||
| std::underlying_type_t<llvm::omp::OpenMPOffloadMappingFlags>>( | ||
| (newDescFlag & | ||
| llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM)) && |
There was a problem hiding this comment.
Would something like this be equivalent? I'm just suggesting it because these if statements are quite difficult to read currently:
| static_cast< | |
| std::underlying_type_t<llvm::omp::OpenMPOffloadMappingFlags>>( | |
| (newDescFlag & | |
| llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM)) && | |
| llvm::to_underlying(newDescFlag & llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM) && |
| @@ -59,6 +59,34 @@ class OMPMapInfoFinalizationPass | |||
| /*corresponding local alloca=*/fir::AllocaOp> | |||
| localBoxAllocas; | |||
|
|
|||
| unsigned long getDescriptorMapType(unsigned long mapTypeFlag, | |||
| mlir::Operation *target) { | |||
| auto newDescFlag = llvm::omp::OpenMPOffloadMappingFlags(mapTypeFlag); | |||
There was a problem hiding this comment.
Nit: To shorten the isa_and_nonnull to isa checks below and improve readability a bit, maybe add here:
if (!target)
return mapTypeFlag;| @@ -2721,6 +2721,24 @@ static void processMapMembersWithParent( | |||
|
|
|||
| assert(memberDataIdx >= 0 && "could not find mapped member of structure"); | |||
|
|
|||
| if (checkIfPointerMap(memberClause)) { | |||
There was a problem hiding this comment.
Nit: Maybe a short descriptive comment of what's achieved by this block here would be nice.
There was a problem hiding this comment.
Happy to add these nits to the main PR set that will eventually override these changes, the upstream PRs can be found here:
So please do review these, I'll aim to port over the comments from here to the PR, but if you wish to also add them inline again on the upstream PR it'd be greatly appreciated!
| op.getMapType().value()), | ||
| builder.getIntegerAttr( | ||
| builder.getIntegerType(64, false), | ||
| getDescriptorMapType(op.getMapType().value(), target)), |
There was a problem hiding this comment.
The map type should never be unset by Flang, which is the only frontend that will use this pass!
Primarily as it saves me replicating the work and risking some disjoint between the end results, but if you feel strongly about any of the comments I am more than happy to make the adjustments to ATD! :-) |
This PR basically adds an additional pointer binding for the descriptor base address. which we attach the user data to, it also makes sure that the mapping for the descriptor is always at a minimum "to" where possible, as regardless of whether or not we wish to map the user data the descriptor wrapping the data must be mapped across as the kernel will still make an attempt to access descriptor information to perform various tasks such as indexing into the user data. This may eventually be restricted a little further in the future to only allow descriptors to have a to mapping in cases where it's legal to have a to mapping (i.e. not legal on an exit directive).