[llvm-branch-commits] [flang] [llvm] [mlir] [OpenMP][MLIR] Modify OpenMP Dialect lowering to support attach mapping (PR #179023)

[via llvm-branch-commits]

================
@@ -4587,10 +4601,16 @@ static void collectMapDataFromMapOperands(
   // Process MapOperands
   for (Value mapValue : mapVars) {
     auto mapOp = cast<omp::MapInfoOp>(mapValue.getDefiningOp());
-    Value offloadPtr =
-        mapOp.getVarPtrPtr() ? mapOp.getVarPtrPtr() : mapOp.getVarPtr();
+    bool isRefPtrOrPteeMapWithAttach =
+        checkRefPtrOrPteeMapWithAttach(mapOp.getMapType());
+    Value offloadPtr = (mapOp.getVarPtrPtr() && !isRefPtrOrPteeMapWithAttach)
----------------
agozillon wrote:

In pretty much every case except the scenarios described in this function it's varPtr, it's only ever anything varPtrPtr related in Fortran when it's a descriptor base address, so previously not too complicated  :-) The new changes are because attach pointers require you to have both the pointer and data address and utilise them both in the mapping to correctly bind things, so it's another scenario where we have to have both varPtr and varPtrPtr present, and alternatively select the varPtr as the offload pointer in the attach case.

I'd personally prefer to just change the varPtr and varPtrPtr field to offload pointer and base address fields, but that might get some push back for diverging from OpenACC. So, the function idea sounds fine by me, but I'd prefer to do that in a subsequent PR as this PR has been in limbo for a while and having to complicate other parts of the PR to simplify this line is a bit counter-productive to trying to synch up downstream and upstream! However, happy to add it in this PR stack if you want me too! I imagine it'd just be an extra addition in the dialect portion, if we don't test this functionality in the LIT infrastructure, it's been a bit since I've had to tinker with anything dialect related.

p.s. the logic for the "?" is actually inlined below for use_device_ptr/addr and has_device_addr, and it's different in these scenarios as the attach maps are elided from these constructs, so we don't need to cover those cases. But in a function scenario it should be fine, it'll just result in an extra bit of bit flag checking for the same results.

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