This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][OMPIRBuilder] Migrate MapCombinedInfoTy from Clang to OpenMPIRBuilder
ClosedPublic

Authored by TIFitis on May 2 2023, 10:24 AM.

Details

Summary

This patch migrates the MapCombinedInfoTy from Clang codegen to OpenMPIRBuilder.

Diff Detail

Event Timeline

TIFitis created this revision.May 2 2023, 10:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2023, 10:24 AM
TIFitis requested review of this revision.May 2 2023, 10:24 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 2 2023, 10:24 AM
jdoerfert added inline comments.May 2 2023, 10:50 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
6848

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.

6872

We should use the base append function, no?

TIFitis marked 2 inline comments as done.May 2 2023, 1:28 PM
TIFitis added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
6848

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.

6872

The base append function is missing append for the members we introduced here like Exprs and Mappers.

TIFitis updated this revision to Diff 518847.May 2 2023, 1:40 PM
TIFitis marked 2 inline comments as done.

Merged MapDevPtrsArrayTy and MapMappersArrayTy into single type named MapValueDeclsArrayTy in CGOpenMPRuntime.cpp

jdoerfert added inline comments.May 2 2023, 5:25 PM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
6848

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?

6872

I understand that. I did not say the base function is sufficient, I said we should use it.

TIFitis updated this revision to Diff 519054.May 3 2023, 6:33 AM

Changed name from llvm::OpenMPIRBuilder::MapCombinedInfoTy to llvm::OpenMPIRBuilder::MapInfosTy, and changed to struct instead of class.

TIFitis marked 2 inline comments as done.May 3 2023, 6:38 AM
TIFitis added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
6848

I have changed the OMPIRBuilder type name to MapInfosTy. Is that okay?

I see what you mean, I have changed it to struct.

6872

Updated to use the base append method.

This revision is now accepted and ready to land.May 3 2023, 5:00 PM
This revision was automatically updated to reflect the committed changes.
TIFitis marked 2 inline comments as done.