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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8021
+  /// the extracted map clauses.
+  void generateAllInfoForMapper(MapBaseValuesArrayTy &BasePointers,
+                                MapValuesArrayTy &Pointers,
----------------
lildmh wrote:
> ABataev wrote:
> > This code has too many common parts with the existing one. Is it possible to merge it somehow or outline into a function?
> I tried to merge it with `generateAllInfo`. The problem is `generateAllInfo` also generates information for clauses including `to, from, is_device_ptr, use_device_ptr`, which don't exist for `declare mapper`. There is no clear way to extract them separately. For example, every 4 or 5 lines, the code is intended to address a different clause type.
> At last, I think the most clear way is to extract all code related to map clauses into this function `generateAllInfoForMapper`. It's ~70 lines of code so not too much.
If those clauses do not exist for the declare mapper, it is fine, no problems with them. If they don't exist, we can't generate anything for them, no?
But if you think, that it would be better to extract some common parts into a separate function, this also works for me.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8530
+      // dynamically.
+      QualType MapArrayType = Ctx.getConstantArrayType(
+          Ctx.getIntTypeForBitwidth(/*DestWidth*/ 64, /*Signed*/ true),
----------------
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.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8588
+
+      if (IsMapper) {
+        // Combine the map type inherited from user-defined mapper with that
----------------
lildmh wrote:
> ABataev wrote:
> > I think it would be good to move this part to the runtime. We should just pass the mapper function to the runtime functions and the runtime should call those mapper functions and get base pointers, pointers, sizes and maptypes.
> This part cannot be moved into the runtime, because the runtime does not know the map type associated with the mapper. Another argument can be potentially added to the runtime API, but that will be more work and I don't think it's necessary
I think it is better again discuss the runtime part of the patch, because everything else depends on the runtime. I would suggest to try to implement the solution we discussed before, where the required data is stored in the runtime dynamic arrays and only after that it is used to transfer the data.



================
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.
----------------
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.


================
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:
> > > > 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.


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

https://reviews.llvm.org/D59474