This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][libomptarget] Add support for declare target to clause under unified memory
ClosedPublic

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

Details

Summary

This patch adds support for handling variables under the:

#pragma omp declare target to()

clause when the

#pragma omp requires unified_shared_memory

is used.

The address of the host variable is copied into the device pointer just like for the declare target link case.

Diff Detail

Repository
rL LLVM

Event Timeline

gtbercea created this revision.Jun 10 2019, 4:42 PM
gtbercea edited the summary of this revision. (Show Details)Jun 10 2019, 4:48 PM
gtbercea added a reviewer: AlexEichenberger.
grokos added inline comments.Jun 14 2019, 11:17 AM
libomptarget/plugins/cuda/src/rtl.cpp
452–453 ↗(On Diff #203934)

Maybe this OR is redundant; it always evaluates to true. Since the symbol we are processing has size!=0 it is a variable and variables are always either "to" or "link", there are no other possibilities (at least now, maybe in the future more attributes will be added).

gtbercea marked an inline comment as done.Jun 14 2019, 11:23 AM
gtbercea added inline comments.
libomptarget/plugins/cuda/src/rtl.cpp
452–453 ↗(On Diff #203934)

So I could remove the attribute check and just have:

if (DeviceInfo.RequiresFlags & OMP_REQ_UNIFIED_SHARED_MEMORY) {..}

Would that work?

The reason I included this explicit check (even if it evaluates to true always) is that if in the future this is extended with other attributes then this check will not automatically work for that. I think this disambiguates the cases that are supported today and guard against this path being taken unintentionally.

grokos accepted this revision.Jun 14 2019, 12:42 PM

LGTM

libomptarget/plugins/cuda/src/rtl.cpp
452–453 ↗(On Diff #203934)

Yes, that's what I thought as well. Let's leave the check there, it makes the implementation more future-proof.

This revision is now accepted and ready to land.Jun 14 2019, 12:42 PM
gtbercea marked 3 inline comments as done.Jun 14 2019, 1:17 PM
gtbercea added inline comments.
libomptarget/plugins/cuda/src/rtl.cpp
452–453 ↗(On Diff #203934)

Awesome. I'll leave it as is for now then.

gtbercea marked an inline comment as done.Jun 14 2019, 1:17 PM
jcownie marked an inline comment as done.Jun 17 2019, 1:36 AM
jcownie added a subscriber: jcownie.
jcownie added inline comments.
libomptarget/plugins/cuda/src/rtl.cpp
452–453 ↗(On Diff #203934)

The code which is checking the flags looks a bit weird. One test is done with & (suggesting these are bit flags which could be or-ed together), the other with == suggesting this is a field full of mutually exclusive enumerations.

grokos added inline comments.Jun 17 2019, 3:00 AM
libomptarget/plugins/cuda/src/rtl.cpp
452–453 ↗(On Diff #203934)

This is because actually "TO" is not a flag, it's the complete absence of flags (0x00). If something has been "declared target" and is not a ctor/dtor or a linked variable then it is "TO".

I don't like this fact either - if we want to be 100% correct we should introduce a dedicated flag for "TO" but that involves changes in the compiler/library interface and is definitely not part of this patch. We can pursue such a patch anyway if there are strong arguments that warrant the effort.

gtbercea updated this revision to Diff 205594.Jun 19 2019, 7:59 AM
  • Fix condition.
gtbercea marked an inline comment as done.Jun 19 2019, 8:00 AM
gtbercea added inline comments.
libomptarget/plugins/cuda/src/rtl.cpp
452–453 ↗(On Diff #203934)

I've move the checks into a comment and simplified the condition.

grokos added inline comments.Jun 19 2019, 8:32 AM
libomptarget/plugins/cuda/src/rtl.cpp
452–453 ↗(On Diff #203934)

This is what I had in mind when I posted the first comment. It looks better now!

gtbercea marked an inline comment as done.Jun 19 2019, 8:39 AM
gtbercea added inline comments.
libomptarget/plugins/cuda/src/rtl.cpp
452–453 ↗(On Diff #203934)

Awesome I'll commit this version!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2019, 8:44 AM