[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/CGDecl.cpp:2533
+                                         CodeGenFunction *CGF) {
+  if (!LangOpts.OpenMP || LangOpts.OpenMPSimd ||
+      (!LangOpts.EmitAllDecls && !D->isUsed()))
----------------
lildmh wrote:
> ABataev wrote:
> > Why do we need to emit it for simd only mode?
> This code is not emitting mapper for simd only mode.
Ah, yes, it is the condition for the early exit.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:7116-7124
+  /// Get the offset of the OMP_MAP_MEMBER_OF field.
+  static unsigned getFlagMemberOffset() {
+    unsigned Offset = 0;
+    for (uint64_t Remain = OMP_MAP_MEMBER_OF; !(Remain & 1);
+         Remain = Remain >> 1)
+      Offset++;
+    return Offset;
----------------
lildmh wrote:
> ABataev wrote:
> > Maybe it is better to define a constant `constexpr uint64_t OMP_MEMBER_OF_RANK = 48` and then deduce `OMP_MAP_MEMBER_OF` as `~((1<<OMP_MEMBER_OF_RANK) - 1)`?
> In libomptarget, the same way is used to define `OMP_TGT_MAPTYPE_MEMBER_OF`: `OMP_TGT_MAPTYPE_MEMBER_OF       = 0xffff000000000000`. So I think they should stay the same. Btw, the number 48 is directly used in libomptarget now, which may need to change in the future.
> 
> In your code, it assumes bits higher than 48 are all `OMP_MAP_MEMBER_OF`, which may not be true in the future. My code here is more universal, although it does not look great. What do you think?
You can apply a mask to drop some of the most significant bits if required. My code looks much cleaner it would be good to have the same code in libomptarget too.
But it is up to you what to do here.



================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8116
+                                MapFlagsArrayTy &Types) const {
+    // FIXME: MSVC 2013 seems to require this-> to find member CurDir.
+    assert(this->CurDir.is<const OMPDeclareMapperDecl *>() &&
----------------
ABataev wrote:
> AFAIK, LLVM has dropped support for msvc 2013, do we still need this?
Ping.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:810-815
+  virtual void emitUserDefinedMapper(const OMPDeclareMapperDecl *D,
+                                     CodeGenFunction *CGF = nullptr);
+
+  /// Emit the array initialization or deletion portion for user-defined mapper
+  /// code generation.
+  virtual void emitUDMapperArrayInitOrDel(
----------------
I don't think we need virtual functions here, non-virtual are good enough.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:2110
+  /// Emit code for the user defined mapper construct.
+  void emitUserDefinedMapper(const OMPDeclareMapperDecl *D,
+                             CodeGenFunction *CGF = nullptr) override;
----------------
I think you can drop this function here if the original function is not virtual


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

https://reviews.llvm.org/D59474