[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:8739
+///     // For each component specified by this mapper:
+///     if (currentComponent.hasMapper())
+///       (*currentComponent.Mapper())(rt_mapper_handle, arg_base, arg_begin,
----------------
lildmh wrote:
> ABataev wrote:
> > Currently `currentComponent` is generated by the compiler. But can we instead pass this data as an extra parameter to this `omp_mapper` function.
> Emm, I think this scheme will be very difficult and inefficient. If we pass components as an argument of `omp_mapper` function, it means that the runtime needs to generate all components related to a map clause. I don't think the runtime is able to do that efficiently. On the other hand, in the current scheme, these components are naturally generated by the compiler, and the runtime only needs to know the base pointer, pointer, type, size. etc.
With the current scheme, we may end with the code blowout. We need to generate very similar code for different types and variables. The worst thing here is that we will be unable to optimize this huge amount of code because the codegen relies on the runtime functions and the code cannot be inlined. That's why I would like to move as much as possible code to the runtime rather than to emit it in the compiler. 


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8740
+///     if (currentComponent.hasMapper())
+///       (*currentComponent.Mapper())(rt_mapper_handle, arg_base, arg_begin,
+///                                    arg_size, arg_type);
----------------
lildmh wrote:
> ABataev wrote:
> > I don't see this part of logic in the code. Could you show me where is it exactly?
> This part doesn't exist in this patch. Currently we don't really look up the mapper for any mapped variable/array/etc. The next patch will add the code to look up the specified mapper for every map clause, and get the mapper function for them correspondingly.
Then we need at least some kind of `TODO` note that this part is not implemented in this patch.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8773
+                             C.VoidPtrTy, ImplicitParamDecl::Other);
+  ImplicitParamDecl SizeArg(C, SizeTy, ImplicitParamDecl::Other);
+  ImplicitParamDecl TypeArg(C, Int64Ty, ImplicitParamDecl::Other);
----------------
Always better to use constructor with the location to generate correct debug info for all the parameters.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8866-8867
+      createRuntimeFunction(OMPRTL__tgt_mapper_num_components), OffloadingArgs);
+  llvm::Value *ShiftedPreviousSize =
+      MapperCGF.Builder.CreateShl(PreviousSize, MapperCGF.Builder.getInt64(48));
+
----------------
I don't like this code very much! It hides the logiс ща the MEMBER_OF flag deep inside and it is going to be very hard to update it in future if there are some changes in the flags.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8934
+        MapperCGF.Builder.getInt64(MappableExprsHandler::OMP_MAP_TO));
+    MapperCGF.Builder.CreateCondBr(IsTo, ToBB, ToElseBB);
+    // In case of to, clear OMP_MAP_FROM.
----------------
I don't see this logic in the comment for the function. Could you add more details for all this logic implemented here?


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8995
+
+// Emit the array initialization or deletion portion for user-defined mapper
+// code generation.
----------------
1. Use `///` style of comment here
2. Add the description of the logic implemented here


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9003
+  QualType SizeTy = C.getSizeType();
+  std::string Prefix = IsInit ? ".init" : ".del";
+
----------------
Use `StringRef` or `SmallString`


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9020-9026
+  if (IsInit)
+    DeleteCond = MapperCGF.Builder.CreateIsNull(
+        DeleteBit, "omp.array" + Prefix + ".delete");
+  else {
+    DeleteCond = MapperCGF.Builder.CreateIsNotNull(
+        DeleteBit, "omp.array" + Prefix + ".delete");
+  }
----------------
Enclose all substatements into braces or none of them.


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

https://reviews.llvm.org/D59474