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

[Lingda Li via Phabricator via cfe-commits]

lildmh marked 3 inline comments as done.
lildmh added inline comments.


================
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;
----------------
ABataev wrote:
> 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.
> 
Maybe let's keep it this way in this patch, then we modify both places in a further patch?


================
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:
> ABataev wrote:
> > AFAIK, LLVM has dropped support for msvc 2013, do we still need this?
> Ping.
I'm okay either way. I guess it doesn't hurt to keep it?


================
Comment at: lib/CodeGen/CGOpenMPRuntime.h:2110
+  /// Emit code for the user defined mapper construct.
+  void emitUserDefinedMapper(const OMPDeclareMapperDecl *D,
+                             CodeGenFunction *CGF = nullptr) override;
----------------
ABataev wrote:
> I think you can drop this function here if the original function is not virtual
The function for simd only mode includes a `llvm_unreachable`, so I think it's still needed as a virtual function above


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

https://reviews.llvm.org/D59474