Page MenuHomePhabricator

[OpenMP][libomptarget] Enable usage of unified memory for declare target link variables
ClosedPublic

Authored by gtbercea on Apr 18 2019, 12:06 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

gtbercea created this revision.Apr 18 2019, 12:06 PM
gtbercea updated this revision to Diff 200560.May 21 2019, 11:46 AM
  • Disable device copies for target link variables under unified memory.
  • Add requires init on plugin side.

Moved some of the functionality from D60223 to here since it fits better with what this patch is trying to achieve.

Hahnfeld added inline comments.
libomptarget/include/omptargetplugin.h
34–36 ↗(On Diff #200560)

According to init_requires_ty, this should have a return type. I think it's a good idea to have that from the beginning, this might be useful in the future without breaking compatibility.

libomptarget/plugins/generic-elf-64bit/src/rtl.cpp
73–75 ↗(On Diff #200560)

As before in D60223: You don't need this here and the implementation is optional, so it can go away.

155–159 ↗(On Diff #200560)

likewise

gtbercea updated this revision to Diff 200771.May 22 2019, 8:59 AM
  • Remove function from generic plugin.
gtbercea marked 3 inline comments as done.May 22 2019, 10:01 AM

Looks mostly good, one last question about the return type of __tgt_rtl_init_requires that I noticed only now.

libomptarget/plugins/cuda/src/rtl.cpp
276 ↗(On Diff #200771)

This will truncate int64_t RequiresFlags to int32_t. Does it make sense to change the return type to int64_t?

grokos added inline comments.May 25 2019, 12:44 PM
libomptarget/plugins/cuda/src/rtl.cpp
276 ↗(On Diff #200771)

I would suggest that we change the return type to 64 bits. In the future we may need to add extra "requires" attributes that go beyond 32 bits and then we will have to modify this interface function. Let's be proactive and avoid such situations. The same thing happened before with map types and device IDs.

gtbercea updated this revision to Diff 201728.May 28 2019, 11:23 AM
  • Change return type to 64 bits.
gtbercea marked 2 inline comments as done.May 28 2019, 11:23 AM
gtbercea marked an inline comment as done.
gtbercea added inline comments.
libomptarget/plugins/cuda/src/rtl.cpp
276 ↗(On Diff #200771)

Agreed. Just updated the patch to reflect that.

grokos added inline comments.May 31 2019, 2:59 PM
libomptarget/plugins/exports
5 ↗(On Diff #201728)

One last question because I'm not very familiar with exporting symbols. If a plugin does not implement __tgt_rtl_init_requires (as is the case with generic-elf-64), will there be any problem with exporting the symbol?

gtbercea marked an inline comment as done.Jun 3 2019, 7:16 AM
gtbercea added inline comments.
libomptarget/plugins/exports
5 ↗(On Diff #201728)

Supporting that method is optional so the function will not get called in that case. In the next chunk of added code you can see that the call to the init_requires() function is guarded:

if (RTL->init_requires)
    RTL->init_requires(RTLRequiresFlags);
gtbercea updated this revision to Diff 202723.Jun 3 2019, 7:39 AM
  • Clean-up code.
grokos added inline comments.Jun 3 2019, 11:30 AM
libomptarget/plugins/exports
5 ↗(On Diff #201728)

My question was different. I meant, while building libomptarget.rtl.x86_64.so the linker is instructed to export a specific symbol from the exports list but this symbol is not defined in that plugin. Will the linker complain about the missing symbol or just ignore it? I suspect it's the latter, but I just want to make sure.

gtbercea marked an inline comment as done.Jun 3 2019, 11:36 AM
gtbercea added inline comments.
libomptarget/plugins/exports
5 ↗(On Diff #201728)

I've never run into that particular issue so I suspect it is ignored.

gtbercea marked an inline comment as done.Jun 3 2019, 12:01 PM
gtbercea added inline comments.
libomptarget/plugins/exports
5 ↗(On Diff #201728)

I commented our the method for the cuda plugin and I didn't get any link errors so it just gets ignored.

grokos accepted this revision.Jun 3 2019, 1:42 PM

OK, there don't seem to be any more comments, looks good.

This revision is now accepted and ready to land.Jun 3 2019, 1:42 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2019, 8:05 AM