ABataev added a comment. In D59474#1490235 <https://reviews.llvm.org/D59474#1490235>, @lildmh wrote:
> Hi Alexey, > > Let's discuss your runtime data mapping scheme next week first. After that we > will continue the review of this. That would be good, thanks! ================ Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8530 + // dynamically. + QualType MapArrayType = Ctx.getConstantArrayType( + Ctx.getIntTypeForBitwidth(/*DestWidth*/ 64, /*Signed*/ true), ---------------- lildmh wrote: > 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. Let's discuss the runtime at first, later we can return to this. ================ Comment at: lib/CodeGen/CGOpenMPRuntime.h:351 + /// is the asynchronous version. + llvm::DenseMap<const OMPDeclareMapperDecl *, + std::pair<llvm::Function *, llvm::Function *>> ---------------- lildmh wrote: > 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. It is necessary, so, please, add it, thanks! 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
