This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][libomptarget] Enable requires flags for target libraries.
ClosedPublic

Authored by gtbercea on Apr 3 2019, 11:51 AM.

Details

Summary

Target link variables are currently implemented by creating a copy of the variables on the device side and unified memory never gets exploited.

When the prgram uses the:

#pragma omp requires unified_shared_memory

directive in conjunction with a declare target link, the linked variable is no longer allocated on the device and the host version is used instead.

This behavior is overridden by performing an explicit mapping.

A Clang side patch is required.

Event Timeline

gtbercea created this revision.Apr 3 2019, 11:51 AM
gtbercea updated this revision to Diff 193558.Apr 3 2019, 11:56 AM
gtbercea edited the summary of this revision. (Show Details)

Remove line.

gtbercea updated this revision to Diff 193559.Apr 3 2019, 11:59 AM
  • Fix comment.
Harbormaster completed remote builds in B30019: Diff 193559.
gtbercea updated this revision to Diff 193562.Apr 3 2019, 12:02 PM
  • Fix comment.
ABataev added inline comments.Apr 3 2019, 12:03 PM
libomptarget/include/omptarget.h
51

Do you really need this flag? Can you use something different? For example, OMP_TGT_MAPTYPE_LITERAL or OMP_TGT_MAPTYPE_PRIVATE?

libomptarget/plugins/cuda/src/rtl.cpp
454

8->sizeof(void*)

gtbercea updated this revision to Diff 193568.Apr 3 2019, 12:17 PM
  • Fix size.
ABataev added inline comments.Apr 3 2019, 12:22 PM
libomptarget/src/rtl.cpp
181

What should we do instead for other platforms?

AlexEichenberger requested changes to this revision.Apr 3 2019, 12:23 PM
AlexEichenberger marked 2 inline comments as done.

see comments

libomptarget/include/omptarget.h
51

I don't believe it is needed, as all is handled while registering the device library. If anything, you could just use the literal to naturally move the host pointer address to the device. This is if you want to avoid going through the indirection already set in place for globals mapped with "link" clause

libomptarget/plugins/cuda/src/rtl.cpp
454

Shouldn't this code be guarded by some "if the runtime is in unified memory mode," and if we see TARGET_LINK and the runtime is not in unified memory mode, should't there be some error as things are likely to go seriously wrong?

libomptarget/src/rtl.cpp
180

Same comment here... if the runtime is in non-unified memory mode, should't it be an error to see a link?

This revision now requires changes to proceed.Apr 3 2019, 12:23 PM
gtbercea updated this revision to Diff 194191.Apr 8 2019, 12:13 PM
  • Update patch.
gtbercea retitled this revision from [OpenMP][libomptarget] Add support for target link variables when unified memory is enabled to [OpenMP][libomptarget] Enable requires flags for target libraries. Support for target link variables when unified memory is enabled.Apr 8 2019, 12:17 PM
AlexEichenberger requested changes to this revision.Apr 8 2019, 2:28 PM

See suggested changes, should be pretty straightforward to do, let me know if you need help

libomptarget/include/omptarget.h
63

I wonder if it would not be wise to add an "undefined" flag. Initially, the internal state would be in undefined state. Then when the RT receives a "register_requires", one of two actions can happen.

If the RT state is in undefined state, it looks if the new requires are compatible with the current device; if yes, it accepts it, if not it either silently "degrade" the support to something supported, or abort.

If the RT state is not in undefined state, then it looks if the new require is compatible with the prior "register_required" emitted state. If incompatibilities are found, it abort.

Alternatively, one can add an additional bit in the RT state that indicates if one or more register requires have been reported. That second approach is cleaner but need one more variable.

libomptarget/plugins/cuda/src/rtl.cpp
234

See comment above, either set to unitialized, or let the state 0, and add additional variable to state "not initialized yet".

libomptarget/src/rtl.cpp
193

compatibility test required... minimally: accept if not previously initialized; refuse if different for any target related flags

This revision now requires changes to proceed.Apr 8 2019, 2:28 PM
gtbercea updated this revision to Diff 194591.Apr 10 2019, 2:30 PM
  • Fix checks and default flag.
gtbercea marked 2 inline comments as done.Apr 10 2019, 2:32 PM
gtbercea updated this revision to Diff 195399.Apr 16 2019, 9:06 AM
  • Update patch.
gtbercea marked 2 inline comments as done.Apr 16 2019, 9:07 AM
ABataev added inline comments.Apr 16 2019, 11:02 AM
libomptarget/include/omptargetplugin.h
35

Maybe it is better to make it target-independent rather than do this only for NVPTX target?

LGTM, just made one minor suggestion

This revision is now accepted and ready to land.Apr 16 2019, 11:12 AM

response to Alexey's comment

libomptarget/include/omptargetplugin.h
35

I thought about this too, but say for one device, there is only unified memory... so we would actually not care about the requires unified.

So it's probably more flexible to have it there, but I am not opposed to take a stricter approach "flag must be consistent regardless of implementation"

Also, it would be good to try to add the tests for this new functionality

ABataev added inline comments.Apr 18 2019, 12:51 PM
libomptarget/plugins/cuda/src/rtl.cpp
115

I think you need to add some (at least basic) support for this in other plugins - generic-elf-64bit.

gtbercea retitled this revision from [OpenMP][libomptarget] Enable requires flags for target libraries. Support for target link variables when unified memory is enabled to [OpenMP][libomptarget] Enable requires flags for target libraries..Apr 25 2019, 11:14 AM
gtbercea updated this revision to Diff 196892.Apr 26 2019, 11:38 AM
  • Add function to generic elf.
gtbercea marked an inline comment as done.Apr 26 2019, 11:38 AM
ABataev added inline comments.Apr 26 2019, 11:53 AM
libomptarget/plugins/generic-elf-64bit/src/rtl.cpp
155

Where are you going to check that the provided RequiresFlags does not match the previously specified one? In this case, I think, the runtime should terminate execution of the program.

gtbercea updated this revision to Diff 196906.Apr 26 2019, 12:36 PM
  • Add basic functionality.
gtbercea marked an inline comment as done.Apr 26 2019, 12:36 PM
gtbercea updated this revision to Diff 196907.Apr 26 2019, 12:40 PM
  • Fix conditions.

Still, I think you can add some basic tests at least to test error messages and the conditions. See llvm/projects/openmp/runtime/test/worksharing/for/kmp_sch_simd_guided.c as an example.

libomptarget/src/rtl.cpp
198

This can be an assert, I don't think that the compiler should call it with this flag

212

Should it be != here and in other places?

215

Better to emit it in terms of OpenMP pragmas and clauses here and in all other places

gtbercea updated this revision to Diff 196910.Apr 26 2019, 12:53 PM
  • Fix messages.
gtbercea marked 3 inline comments as done.Apr 26 2019, 12:54 PM
gtbercea updated this revision to Diff 196921.Apr 26 2019, 3:39 PM
  • Add test.
gtbercea updated this revision to Diff 196924.Apr 26 2019, 3:44 PM
  • Clean test.

Tests for error messages

grokos added inline comments.May 17 2019, 12:56 PM
libomptarget/src/device.h
107

0 -> OMP_REQ_UNDEFINED?

gtbercea marked 2 inline comments as done.May 17 2019, 1:55 PM
gtbercea added inline comments.
libomptarget/src/device.h
107

OMP_REQ_UNDEFINED is not in scope here and not needed. This 0 never gets used. In rtl.cpp this field will be overwritten with OMP_REQ_UNDEFINED if applicable.

gtbercea marked an inline comment as done.May 17 2019, 1:55 PM
gtbercea updated this revision to Diff 200107.May 17 2019, 3:26 PM
  • Fix test.
gtbercea updated this revision to Diff 200110.May 17 2019, 3:38 PM
  • Add comment.

In this patch __tgt_rtl_init_requires does nothing. Do we really want to add the prototype now? If yes, are we sure that we won't need to pass more information in the future? Once we add it, the runtime library must maintain compatibility.

Moreover, I'm not sure if we should require plugins to implement the function if they don't need any addition checks. I'd propose to make this optional, ie look for the symbol via dlsym and disable the call if it's not found.

libomptarget/test/offloading/requires.c
27–32

This will only work if the test compiler already supports requires. What's wrong with calling __tgt_register_requires twice as I suggested offline?

gtbercea updated this revision to Diff 200326.May 20 2019, 10:07 AM
gtbercea marked an inline comment as done.
  • Fix test to make it compatible with previous clang versions.
gtbercea marked an inline comment as done.May 20 2019, 10:08 AM
gtbercea updated this revision to Diff 200344.May 20 2019, 12:28 PM
  • Make implementation of init requires optional.

In this patch __tgt_rtl_init_requires does nothing. Do we really want to add the prototype now? If yes, are we sure that we won't need to pass more information in the future? Once we add it, the runtime library must maintain compatibility.

Moreover, I'm not sure if we should require plugins to implement the function if they don't need any addition checks. I'd propose to make this optional, ie look for the symbol via dlsym and disable the call if it's not found.

Should be optional now.

grokos accepted this revision.May 20 2019, 12:29 PM

I don't have any other comments or suggestions, the patch looks good.

In this patch __tgt_rtl_init_requires does nothing. Do we really want to add the prototype now? If yes, are we sure that we won't need to pass more information in the future? Once we add it, the runtime library must maintain compatibility.

Ping. At the moment this patch just stores the RequiresFlags in the plugins and never uses it.

libomptarget/test/offloading/requires.c
26–27

Could you please run clang-format on this file?

In this patch __tgt_rtl_init_requires does nothing. Do we really want to add the prototype now? If yes, are we sure that we won't need to pass more information in the future? Once we add it, the runtime library must maintain compatibility.

Ping. At the moment this patch just stores the RequiresFlags in the plugins and never uses it.

If you check the declare target link patch it will be used there. I can move this part of the code to that patch.

gtbercea updated this revision to Diff 200522.May 21 2019, 8:55 AM
  • Format test.
gtbercea updated this revision to Diff 200557.May 21 2019, 11:31 AM
  • Remove requires init on plugin side.

@Hahnfeld just updated the patch. Please let me know if you have any more comments.

Hahnfeld accepted this revision.May 21 2019, 11:48 AM

LG in case we don't need __tgt_register_requires to return a code in the near future (like for saying "that's not supported")

LG in case we don't need __tgt_register_requires to return a code in the near future (like for saying "that's not supported")

Sorry, mixed this up with __tgt_rtl_init_requires which is now gone. LGTM without restrictions :)

libomptarget/src/rtl.h
128–132

Nit: This shouldn't be a copy of the comment for RegisterLib

This revision was automatically updated to reflect the committed changes.
gtbercea marked an inline comment as done.May 21 2019, 12:32 PM