TIFitis marked 2 inline comments as done. TIFitis added inline comments.
================ Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:6848 + class MapCombinedInfoTy : public llvm::OpenMPIRBuilder::MapCombinedInfoTy { + public: MapExprsArrayTy Exprs; ---------------- jdoerfert wrote: > Not sure why you made it a class with public, but I guess it doesn't matter. > Do we really want to use the same name though? I would add "Base" or sth to > the llvm class. The clang code, and eventually the OMPIRBuilder code accesses the members directly so they need to be public. I have kept it the same name in similar fashion to the TargetDataInfo type that also got migrated. From OMPBuilder's perspective the type is sufficient and when using it from MLIR I don't think we would need the extra data structures that clang uses. Let me know what you think, I can change it to a different name as you suggested. ================ Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:6872 NonContigInfo.Strides.append(CurInfo.NonContigInfo.Strides.begin(), CurInfo.NonContigInfo.Strides.end()); } ---------------- jdoerfert wrote: > We should use the base append function, no? The base append function is missing append for the members we introduced here like `Exprs` and `Mappers`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149666/new/ https://reviews.llvm.org/D149666 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits