@@ -399,18 +399,24 @@ class MapInfoFinalizationPass
399399 baseAddrIndex);
400400 }
401401
402- // / Adjust the descriptor's map type such that we ensure the descriptor
403- // / itself is present on device when needed, without changing the user's
404- // / requested data mapping semantics for the underlying data.
405- // /
406- // / We conservatively transform descriptor mappings to `OMP_MAP_TO` (and
407- // / preserve `IMPLICIT`/`ALWAYS` when present) for structured regions. The
408- // / descriptor should live on device for indexing, bounds, etc., but we do
409- // / not require, nor want, additional mapping semantics like `CLOSE` for the
410- // / descriptor entry itself. `CLOSE` (and other user-provided flags) should
411- // / apply to the base data entry that actually carries the pointee, which is
412- // / generated separately as a member map. For `target exit`/`target update`
413- // / we keep the original map type unchanged.
402+ // / Adjusts the descriptor's map type. The main alteration that is done
403+ // / currently is transforming the map type to `OMP_MAP_TO` where possible.
404+ // / This is because we will always need to map the descriptor to device
405+ // / (or at the very least it seems to be the case currently with the
406+ // / current lowered kernel IR), as without the appropriate descriptor
407+ // / information on the device there is a risk of the kernel IR
408+ // / requesting for various data that will not have been copied to
409+ // / perform things like indexing. This can cause segfaults and
410+ // / memory access errors. However, we do not need this data mapped
411+ // / back to the host from the device, as per the OpenMP spec we cannot alter
412+ // / the data via resizing or deletion on the device. Discarding any
413+ // / descriptor alterations via no map back is reasonable (and required
414+ // / for certain segments of descriptor data like the type descriptor that are
415+ // / global constants). This alteration is only inapplicable to `target exit`
416+ // / and `target update` currently, and that's due to `target exit` not
417+ // / allowing `to` mappings, and `target update` not allowing both `to` and
418+ // / `from` simultaneously. We currently try to maintain the `implicit` flag
419+ // / where necessary, although it does not seem strictly required.
414420 unsigned long getDescriptorMapType (unsigned long mapTypeFlag,
415421 mlir::Operation *target) {
416422 using mapFlags = llvm::omp::OpenMPOffloadMappingFlags;
@@ -421,7 +427,6 @@ class MapInfoFinalizationPass
421427 mapFlags flags = mapFlags::OMP_MAP_TO |
422428 (mapFlags (mapTypeFlag) &
423429 (mapFlags::OMP_MAP_IMPLICIT | mapFlags::OMP_MAP_ALWAYS));
424-
425430 // For unified_shared_memory, we additionally add `CLOSE` on the descriptor
426431 // to ensure device-local placement where required by tests relying on USM +
427432 // close semantics.
0 commit comments