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

Reply via email to