[PATCH] D59474: [OpenMP 5.0] Codegen support for user-defined mappers

[Lingda Li via Phabricator via cfe-commits]

lildmh marked 3 inline comments as done.
lildmh added a comment.

Hi Alexey,

Let's discuss your runtime data mapping scheme next week first. After that we will continue the review of this.



================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8530
+      // dynamically.
+      QualType MapArrayType = Ctx.getConstantArrayType(
+          Ctx.getIntTypeForBitwidth(/*DestWidth*/ 64, /*Signed*/ true),
----------------
ABataev wrote:
> lildmh wrote:
> > ABataev wrote:
> > > Hmm, how could you calculate the required size of the array for the mapper? Or this is constant?
> > I'm not sure I understand your question here.
> > Do you mean the size when an OpenMP array section is mapped? If that's the case, it is not constant. Existing code can already handle it.
> > Or do you mean the size of mapper array (i.e., `MapArrayType`)? This is constant and depends on how many map clauses exist in the declare mapper directive.
> Yes, it is the question about the size of mapper array. It is the part of our discussion about mappers generation we had before. You said that it is hard to generate the sizes of the arrays since we don't know the number of map clauses at the codegen phase. bu there we have this number.
Sorry there was probably some miscommunication. What I meant is that after fully expanded, for example, from `map(mapper(id):a[0:n])`, eventually to `map(a.b.c[0:e]) map(a.k) ...`, the number of things in the results is unknown at compile time.

Here, we only do one level of expansion of one instance based on the `declare mapper` directive, for example, the mapper is `declare mapper(class A a) map(a.b[0:a.n]) map(a.c)`
In this case, the size of mapper array is 2, because there are 2 map clauses (actually it's more than 2 because the first map clause maps an array). This number can be decided at compile time easily.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8795
+  // Generate an asynchronous mapper function.
+  llvm::Function *AsyncFn = emitUDMapperFunc(D, /*NoWait=*/true);
+  // Add the generated mapper functions to UDMMap.
----------------
ABataev wrote:
> lildmh wrote:
> > ABataev wrote:
> > > With the buffering-based implementation we need only function.
> > Yes, in either case, we only generate functions here. Is there a problem?
> Sorry, I meant we'll need only one function.
Yes, in that case, only one is needed.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:351
+  /// is the asynchronous version.
+  llvm::DenseMap<const OMPDeclareMapperDecl *,
+                 std::pair<llvm::Function *, llvm::Function *>>
----------------
ABataev wrote:
> lildmh wrote:
> > ABataev wrote:
> > > lildmh wrote:
> > > > ABataev wrote:
> > > > > You should be very careful with this map. If the mapper is declared in the function context, it must be removed from this map as soon as the function processing is completed. All local declarations are removed after this and their address might be used again.
> > > > Yes, I plan to have this part in the next patch, which will implement to look up the corresponding mapper function for map. to, from clauses
> > > Noope, this must be the part of this patch, because it may cause the crash of the compiler during testing.
> > It will not crash the compiler, because this UDMap is only written in this patch, never read.
> Still, you should clear it in this patch. Otherwise, you're breaking data-dependency between the patches and this is not good at all.
Sure, if you think that's absolutely necessary, I can add it to this patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59474/new/

https://reviews.llvm.org/D59474