jdoerfert added inline comments.
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:6848
+ class MapCombinedInfoTy : public llvm::OpenMPIRBuilder::MapCombinedInfoTy {
+ public:
MapExprsArrayTy Exprs;
----------------
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?
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:6872
NonContigInfo.Strides.append(CurInfo.NonContigInfo.Strides.begin(),
CurInfo.NonContigInfo.Strides.end());
}
----------------
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.
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
