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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
