Page MenuHomePhabricator

[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

Repository
rL LLVM

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
2297–2319 ↗(On Diff #203936)

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
7458–7467 ↗(On Diff #203936)

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 ↗(On Diff #203936)

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

test/OpenMP/nvptx_target_requires_unified_shared_memory.cpp
2 ↗(On Diff #203936)

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

2 ↗(On Diff #203936)

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 ↗(On Diff #205667)

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 ↗(On Diff #205667)

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 ↗(On Diff #205667)

Better to have if-else here

9270 ↗(On Diff #205667)

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

lib/CodeGen/CGOpenMPRuntime.h
1125 ↗(On Diff #205667)

Better to name it getAddrOfDeclareTargetVar

lib/CodeGen/CodeGenModule.cpp
2483–2485 ↗(On Diff #205667)

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
9227–9229 ↗(On Diff #205825)

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

lib/CodeGen/CGOpenMPRuntime.h
1125 ↗(On Diff #205667)

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