This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Migrate device code privatisation from Clang CodeGen to OMPIRBuilder
ClosedPublic

Authored by TIFitis on Jun 9 2023, 9:38 AM.

Details

Summary

This patch migrates the UseDevicePtr and UseDeviceAddr clause related code for handling privatisation from Clang codegen to the OMPIRBuilder

Depends on D150860

Diff Detail

Event Timeline

TIFitis created this revision.Jun 9 2023, 9:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2023, 9:38 AM
TIFitis requested review of this revision.Jun 9 2023, 9:38 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 9 2023, 9:38 AM
TIFitis added inline comments.Jun 9 2023, 9:51 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
7186–7250

Currently clang maintains a map between Clang::ValueDecl and corresponding address in Info.CaptureDeviceAddrMap.

Please see the EmitOMPUseDeviceAddrClause function just below this to see how it does so.

I would however like to get rid of this map as it allows us to then get rid of another map in the MapInfosTy and a clang specific callback for maintaining these maps..

This map is only used to retain the mapping between a ValueDecl inside a map clause to its corresponding llvm address.

The highlighted code here tries to retrieve this llvm address back from the ValueDecl without using the map, but as you can it is a very hacky way of doing so. But my intuition says that there must be a much simpler method of retrieving this address as Clang must be storing it in some way in order for it to generate code.

It would be great if someone with more experience in Clang codegen could help me achieve this.

Also I am aware that I am probably doing a very poor job at explaining this. Please let me know if you have any questions or would like me to try and explain it with more clarity.

TIFitis updated this revision to Diff 531401.Jun 14 2023, 9:55 AM

Fixed typo

typo in the title.

clang/lib/CodeGen/CGOpenMPRuntime.h
1463

If it is an alloca (for sure) use llvm::AllocaInst.

clang/test/OpenMP/target_data_use_device_ptr_codegen.cpp
418

Did we just change the order or is this non-deterministic? The latter is not acceptable, the former might be.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
4217

What else could it be here?

4787

What if the PointerBCOrASCast and the GEP are both no-ops, and BPVal is an alloca. Would that cause a problem as we later only check if the type of the value is an alloca. That said, it seems dangerous to only rely on the type of the value.

TIFitis updated this revision to Diff 536226.Jun 30 2023, 6:59 AM
TIFitis marked 4 inline comments as done.

Addressed reviewer comments.

clang/lib/CodeGen/CGOpenMPRuntime.h
1463

It's alloca for use_dev_ptr and usually GEPInst for use_dev_addr.
Updated comment to llvm address

clang/lib/CodeGen/CGStmtOpenMP.cpp
7186–7250

Please ignore the above comment. I've kept things simpler for this patch.

I'll try to have another patch in the future to focus on removing the clang specific maps and callbacks.

clang/test/OpenMP/target_data_use_device_ptr_codegen.cpp
418

It's a change of order. I've used a llvm::SmallMapVector for determinism.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
4217

For use_dev_ptr a new alloca is inserted.
For use_dev_addr it points to the GEP instruction from the mappers.

4787

I don't think the GEP can be a no-op here as it's from the mapper array that we are adding above.

That being said, at worst we would be emitting a spurious load and store instruction with no use.

TIFitis retitled this revision from [OpenMP] Migrate deviice code privatization from Clang CodeGen to OMPIRBuilder to [OpenMP] Migrate device code privatisation from Clang CodeGen to OMPIRBuilder.Jun 30 2023, 7:00 AM
TIFitis edited the summary of this revision. (Show Details)

Ping for review :)

This revision is now accepted and ready to land.Jul 11 2023, 10:51 AM
This revision was landed with ongoing or failed builds.Jul 12 2023, 4:03 AM
This revision was automatically updated to reflect the committed changes.