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: > TIFitis wrote: > > 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. > Two structures with the same name is not a good idea. Rename one. > > struct A === class A + public, > so why did you move from struct A to class A + public? I have changed the OMPIRBuilder type name to MapInfosTy. Is that okay? I see what you mean, I have changed it to struct. ================ Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:6872 NonContigInfo.Strides.append(CurInfo.NonContigInfo.Strides.begin(), CurInfo.NonContigInfo.Strides.end()); } ---------------- jdoerfert wrote: > TIFitis wrote: > > 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`. > I understand that. I did not say the base function is sufficient, I said we > should use it. > Updated to use the base append method. 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