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

[Lingda Li via Phabricator via cfe-commits]

lildmh 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,
----------------
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.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8740
+///     if (currentComponent.hasMapper())
+///       (*currentComponent.Mapper())(rt_mapper_handle, arg_base, arg_begin,
+///                                    arg_size, arg_type);
----------------
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.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8785
+  std::string Name = getName({"omp_mapper", Ty.getAsString(), D->getName()});
+  std::replace(Name.begin(), Name.end(), ' ', '_');
+  auto *Fn = llvm::Function::Create(FnTy, llvm::GlobalValue::InternalLinkage,
----------------
ABataev wrote:
> Bad idea to do this. Better to use something like this:
> ```
> SmallString<256> TyStr;
> llvm::raw_svector_ostream Out(TyStr);
> CGM.getCXXABI().getMangleContext().mangleTypeName(Ty, Out);
> 
> ```
> 
Sounds good. Thanks!


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

https://reviews.llvm.org/D59474