================
@@ -58,21 +67,62 @@ class MapInfoFinalizationPass
            /*corresponding local alloca=*/fir::AllocaOp>
       localBoxAllocas;
 
-  void genDescriptorMemberMaps(mlir::omp::MapInfoOp op,
-                               fir::FirOpBuilder &builder,
-                               mlir::Operation *target) {
-    mlir::Location loc = op.getLoc();
-    mlir::Value descriptor = op.getVarPtr();
+  /// getMemberUserList gathers all users of a particular MapInfoOp that are
+  /// other MapInfoOp's and places them into the mapMemberUsers list, which
+  /// records the map that the current argument MapInfoOp "op" is part of
+  /// alongside the placement of "op" in the recorded users members list. The
+  /// intent of the generated list is to find all MapInfoOp's that may be
+  /// considered parents of the passed in "op" and in which it shows up in the
+  /// member list, alongside collecting the placement information of "op" in 
its
+  /// parents member list.
+  void
+  getMemberUserList(mlir::omp::MapInfoOp op,
+                    llvm::SmallVectorImpl<ParentAndPlacement> &mapMemberUsers) 
{
----------------
agozillon wrote:

I believe I answered some variation of this question in the previous PR 
iteration, so just going to copy paste it here: 

"it's intentional for the moment, there should never be more than one 
mapMemberUsers currently when we reach this pass, as we do not generate MLIR 
that allows the re-use of MapInfoOp's across operations, so technically the 
only cases where a MapInfoOp should be used, is a parent map member operation 
(in which case we add an entry to mapMemberUsers) or when it's tied to an 
existing MapInfoOp using operation (TargetOp/TargetExitData etc.).

So, perhaps being a bit pre-emptive and adding some initial support for this 
case, for when we may end up trying to generating tidier MLIR where we re-use 
map info operations allowing them to have multiple users. As that would be 
ideal in the future (and I believe you've already written one such test 
previously).

Previously an assert checking that we had no more than 1 user was above but i 
think it is covered by the prior assert at line 488 so it felt a little 
redundant to have two of the same check."

I believe there's another comment at line 271-ish (after updates may be off by 
a bit) that mentions the above, and I believe I'll be re-adding the assert as 
requested by @skatrak :-) 

https://github.com/llvm/llvm-project/pull/111192
_______________________________________________
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

Reply via email to