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

[Lingda Li via Phabricator via cfe-commits]

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

Hi Alexey,

Again, thanks for your review! Sorry I didn't get back to you in time because I was distracted by other things. Please see the comments inline.



================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8021
+  /// the extracted map clauses.
+  void generateAllInfoForMapper(MapBaseValuesArrayTy &BasePointers,
+                                MapValuesArrayTy &Pointers,
----------------
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.


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


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


================
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:
> With the buffering-based implementation we need only function.
Yes, in either case, we only generate functions here. Is there a problem?


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


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

https://reviews.llvm.org/D59474