[clang] [llvm] [Clang][OpenMP] Capture mapped pointers on `target` by reference. (PR #145454)

[Alexey Bataev via cfe-commits]

================
@@ -8811,8 +8829,19 @@ class MappableExprsHandler {
         ++EI;
       }
     }
-    llvm::stable_sort(DeclComponentLists, [](const MapData &LHS,
-                                             const MapData &RHS) {
+    llvm::stable_sort(DeclComponentLists, [VD](const MapData &LHS,
+                                               const MapData &RHS) {
+      // For cases like map(p, p[0], p[0][0]), the shortest map, like map(p)
+      // in this case, should be handled first, to ensure that it gets the
+      // TARGET_PARAM flag.
+      OMPClauseMappableExprCommon::MappableExprComponentListRef Components =
+          std::get<0>(LHS);
+      OMPClauseMappableExprCommon::MappableExprComponentListRef ComponentsR =
+          std::get<0>(RHS);
+      if (VD && VD->getType()->isAnyPointerType() && Components.size() == 1 &&
+          ComponentsR.size() > 1)
+        return true;
----------------
alexey-bataev wrote:

> Is there a case you have in mind? If the number of components is same, then this should not return anything, and we continue to the existing logic and return based on that in line 8855.
> 
> The new return should only kick-in if the current component list has one entry and the other one has more.
> 
> I originally wanted to use the following, which is broader, (and the comment still is in line with this):
> 
> ```c++
> if (VD && VD->getType()->isAnyPointerType() && Components.size() !=
>           ComponentsR.size())
>   return Components.size() < ComponentsR.size();
> ```
> 
> because even when we have `map(p[0][0], p[0])`, the `map(p[0])` should contribute to the kernel argument. `p` is predetermined `firstprivate because `p`is the base-pointer of`map(p[0])`.

I think this one works better.

> 
> The base-pointer for `int **p; ... map(p[0][0])` is `p[0]`, so it has no effect on the data-sharing/mapping of `p`, and hence should not determine the kernel argument.
> 
> But technically we don't need to sort between `p[0][0]` and `p[0][0][0]` since neither of them should ideally contribute to the determination of the data-sharing/mapping clause on `p`.
> 
> Note that sorting between `p[0]` and `p[0][0]` is not needed _yet_, since both `p[0][0]` and `p[0]` use PTR_AND_OBJ mappings that start with `p`.
> 
> Future Plans:
> 
> I am working on a change to support OpenMP 5.0/6.1 compliant pointer-attachment that avoids using the PTR_AND_OBJ mapping for them, and instead uses a new ATTACH map-type bit. With that, the map-chain emitted for `map(present, to: p[0][0]) map(from: p[0])` would look like the following:
> 
> ```c
> // For p[0][0]
> &p[0][0], &p[0][0], sizeof(p[0][0]), TO | PRESENT
> &p[0], &p[0][0], sizeof(ptr), ATTACH
> 
> // For p[0]
> &p[0], &p[0], sizeof(p[0]), FROM | PARAM
> &p, &p[0], ATTACH
> 
> ATTACH:
> * New map-type bit which means don't increase the ref-count, only do an attachment iff either `&p`'s ref-count is 1, or `&p[0]`'s ref-count is 1 after finishing all maps, including mappers.
> * Can be combined with ALWAYS for `attach(always)` map-type-modifier to force the attachment even if the ref-counts are more than 1.
> * Can be removed if the user says `attach(never)` (OpenMP 6.1)
> ```
> 
> Once the above is supported, we would need the PARAM bit to apply to the map of `p[0]`, which would likely require sorting between the component-lists, even if neither is of size 1.



https://github.com/llvm/llvm-project/pull/145454