This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Add support for handling declare target to clause when unified memory is required
ClosedPublic

Authored by gtbercea on Jun 10 2019, 4:47 PM.

Details

Summary

This patch adds support for the handling of the variables under the declare target to clause.

The variables in this case are handled like link variables are. A pointer is created on the host and then mapped to the device. The runtime will then copy the address of the host variable in the device pointer.

Diff Detail

Event Timeline

gtbercea created this revision.Jun 10 2019, 4:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2019, 4:47 PM

Will review it next week, when I'm back to work, need to think about it a little bit.

ABataev added inline comments.Jun 19 2019, 11:26 AM
lib/CodeGen/CGExpr.cpp
2298–2320

I think it would be better to merge these 2 functions into 1 emitDeclTargetVarDeclLValue. It should return the correct address for link vars and to vars with unified memory.

lib/CodeGen/CGOpenMPRuntime.cpp
7494–7503

You can merge those pieces of code into one, no need to duplicate. Plus, I don't think we need a new flag for to with unified memory if we can express everything using the existing flags.

lib/CodeGen/CGOpenMPRuntime.h
1123–1127

Same here, better to merge these 2 functions into one getAddrOfDeclareTargetVar

test/OpenMP/nvptx_target_requires_unified_shared_memory.cpp
2

CHECK is the default prefix, no need to specify it.

2

We also need the checks for the device codegen since you're changing something not only in the host codegen, but in the device codegen too. Just extend this test to check for the codegen for the device.

gtbercea updated this revision to Diff 205667.Jun 19 2019, 2:09 PM
gtbercea marked an inline comment as done.
  • Merge MT_Link and MT_To with unified memory cases.
  • Transform switch into if statements.
  • Fix declare target attribute checks.
gtbercea marked 3 inline comments as done.Jun 19 2019, 2:10 PM

Still need to update the test but the rest of the code is updated.

ABataev added inline comments.Jun 20 2019, 7:38 AM
lib/CodeGen/CGOpenMPRuntime.cpp
2566–2569

I think we can use the same suffix in all cases, instead of _link_ptr or _to_ptr just something like _ref_ptr would be good.

7465

Fix the comment too. Also, the name of the variable is not very good, better to rename it to something like MustBeReference or something similar.

9230–9232

Better to have if-else here

9270

|| (*Res == OMPDeclareTargetDeclAttr::MT_To && HasRequiresUnifiedSharedMemory) and fix the message too

lib/CodeGen/CGOpenMPRuntime.h
1125

Better to name it getAddrOfDeclareTargetVar

lib/CodeGen/CodeGenModule.cpp
2483–2485

Better to keep the assert but with a different condition.

gtbercea updated this revision to Diff 205825.Jun 20 2019, 8:44 AM
  • Address comments.
gtbercea marked 6 inline comments as done.Jun 20 2019, 8:45 AM
gtbercea marked an inline comment as done.Jun 20 2019, 9:01 AM
gtbercea marked an inline comment as done.
ABataev added inline comments.Jun 20 2019, 9:14 AM
lib/CodeGen/CGOpenMPRuntime.cpp
9230–9234

Use assert here instead of else if and remove else with llvm_unreachable

lib/CodeGen/CGOpenMPRuntime.h
1125

Not renamed

gtbercea updated this revision to Diff 205839.Jun 20 2019, 9:25 AM
  • Address comments.
This revision is now accepted and ready to land.Jun 20 2019, 9:28 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2019, 11:02 AM