Page MenuHomePhabricator

[OpenMP][libomptarget] Add data sharing support in libomptarget
ClosedPublic

Authored by gtbercea on Dec 21 2017, 4:29 AM.

Details

Summary

This patch extends the libomptarget functionality in patch D14254 with support for the data sharing scheme for supporting implicitly shared variables. The runtime therefore maintains a list of references to shared variables.

Diff Detail

Repository
rOMP OpenMP

Event Timeline

gtbercea created this revision.Dec 21 2017, 4:29 AM
Hahnfeld added inline comments.Dec 21 2017, 4:37 AM
libomptarget/deviceRTLs/nvptx/src/interface.h
472–477

We have to agree on an order here: In D41012 I chose to put IsOMPRuntimeInitialized last which matches the other functions.

Hahnfeld edited subscribers, added: openmp-commits; removed: cfe-commits.Dec 21 2017, 4:39 AM
gtbercea added inline comments.Dec 21 2017, 5:11 AM
libomptarget/deviceRTLs/nvptx/src/interface.h
472–477

Is it ok if we go with the order in this patch?

Hahnfeld added inline comments.Dec 21 2017, 6:02 AM
libomptarget/deviceRTLs/nvptx/src/interface.h
472–477

In the end I don't really care, but IMO it's more consistent to have IsOMPRuntimeInitialized last in the list of arguments. That's the reason I submitted D41012 as it currently is.

gtbercea added inline comments.Dec 22 2017, 3:18 AM
libomptarget/deviceRTLs/nvptx/src/interface.h
472–477

Great! Let's use yours then and I can change the order of the args in this patch. Can you make sure you push the other patch and then once this one lands it should all work.

gtbercea updated this revision to Diff 128004.Dec 22 2017, 3:29 AM
Hahnfeld requested changes to this revision.Jan 28 2018, 2:23 AM

Please rebase on top of D14254 and run clang-format.

libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.cu
35

I'm not sure if this needs to be defined here... Hopefully this lightens up after rebasing ;-)

This revision now requires changes to proceed.Jan 28 2018, 2:23 AM
Hahnfeld added inline comments.Jan 30 2018, 9:07 AM
libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.cu
35

Yeah, I think this needs be be declared extern here and defined in omp_data.cu.

Hahnfeld added inline comments.Jan 30 2018, 9:21 AM
libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.cu
35

Actually, all of these extern declarations should go away because they already exist in omptarget-nvptx.h! We don't need to reiterate them here...

gtbercea updated this revision to Diff 133061.Feb 6 2018, 12:21 PM

Update against latest master branch.

grokos accepted this revision.Feb 6 2018, 12:32 PM

Looks good. Thanks for submitting! I'll take care of duplicate extern declarations in another patch.

Hahnfeld added inline comments.Feb 6 2018, 12:51 PM
libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.cu
35

I still think this won't work: If there are multiple translation units with target regions, the linker should complain about duplicate symbol definitions... I think that's why there is a separate file omp_data.cu.

Hahnfeld added inline comments.Feb 6 2018, 1:00 PM
libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.cu
35

(obviously only when building and using the bclib which we don't yet have in trunk; but I think we shouldn't introduce the problem if we can easily avoid it)

gtbercea marked an inline comment as done.Feb 7 2018, 7:31 AM
gtbercea added inline comments.
libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.cu
35

Do you have a failing example for this?

Also I'm not sure what you want me to move where. The comments keep going back and forth.

Hahnfeld added inline comments.Feb 7 2018, 7:50 AM
libomptarget/deviceRTLs/nvptx/src/omptarget-nvptx.cu
35

Compiling and linking multiple source files with target regions should fail when using the bclib.

Just move the definition (ie without extern) to omp_data.cu. (Terminology taken from https://stackoverflow.com/a/1410632)

gtbercea updated this revision to Diff 133219.Feb 7 2018, 8:09 AM

Move definition of data sharing variables.

gtbercea marked 2 inline comments as done.Feb 7 2018, 8:10 AM

@Hahnfeld any further comments?

Hahnfeld accepted this revision.Feb 7 2018, 9:07 AM

@Hahnfeld any further comments?

No, look good!

This revision is now accepted and ready to land.Feb 7 2018, 9:07 AM
This revision was automatically updated to reflect the committed changes.