[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,
----------------
This code has too many common parts with the existing one. Is it possible to merge it somehow or outline into a function?


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8530
+      // dynamically.
+      QualType MapArrayType = Ctx.getConstantArrayType(
+          Ctx.getIntTypeForBitwidth(/*DestWidth*/ 64, /*Signed*/ true),
----------------
Hmm, how could you calculate the required size of the array for the mapper? Or this is constant?


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8588
+
+      if (IsMapper) {
+        // Combine the map type inherited from user-defined mapper with that
----------------
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.


================
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.
----------------
With the buffering-based implementation we need only 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:
> > 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.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:800
+  /// Emit a function for a user defined mapper. Whether it is synchronous or
+  /// asynchronous depends on \a NoWait.
+  virtual llvm::Function *emitUDMapperFunc(const OMPDeclareMapperDecl *D,
----------------
I don't remember precisely, but probably `\a`->`\p`


================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:804
+
+  // Emit the array initialization or deletion portion for user-defined mapper
+  // code generation.
----------------
Use `///` style of comment


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

https://reviews.llvm.org/D59474