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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
