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

[Lingda Li via Phabricator via cfe-commits]

lildmh marked an inline comment as done.
lildmh added inline comments.


================
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:
> lildmh wrote:
> > lildmh wrote:
> > > ABataev wrote:
> > > > ABataev wrote:
> > > > > lildmh wrote:
> > > > > > 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
> > > > > It is better to reduce number of virtual functions, if possible.
> > > > What about virtual function?
> > > Since `emitUserDefinedMapper` here overrides the same function in `CGOpenMPRuntime`, I think `emitUserDefinedMapper` of `CGOpenMPRuntime` needs to be defined as `virtual`?
> > If you are asking about what virtual function I removed, it is `emitUDMapperArrayInitOrDel` of `CGOpenMPRuntime` which is never overrided.
> I would suggest to not make this virtual too and remove overriden version. It does not help.
If we remove virtual from `emitUserDefinedMapper`, we will have to define it in both `CGOpenMPRuntime` and `CGOpenMPSIMDRuntime`, and its definition is identical in both places. I don't think that's good software engineering practice?


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

https://reviews.llvm.org/D59474