Page MenuHomePhabricator

Add omp_get_device_num() and update several other device API functions
ClosedPublic

Authored by tlwilmar on Nov 9 2018, 12:11 PM.

Details

Summary

Add omp_get_device_num() function for 5.0 which returns the number of the device the current thread is running on. Also, did some cleanup and updating of device API functions to make them into weak functions that should be replaced with libomptarget functions when libomptarget is present.

Diff Detail

Repository
rL LLVM

Event Timeline

tlwilmar created this revision.Nov 9 2018, 12:11 PM
jlpeyton accepted this revision.Nov 28 2018, 12:11 PM
jlpeyton added a reviewer: jlpeyton.
jlpeyton added a subscriber: jlpeyton.

LGTM

This revision was not accepted when it landed; it landed in state Needs Review.Nov 28 2018, 12:13 PM
This revision was automatically updated to reflect the committed changes.

This commit conflicts with libomptarget. libomptarget have the same functions and the code may be confused during execution which version of the functions should be executed. This conflict should be resolved somehow. Before that, I think, this patch must be reverted.

This commit conflicts with libomptarget. libomptarget have the same functions and the code may be confused during execution which version of the functions should be executed. This conflict should be resolved somehow. Before that, I think, this patch must be reverted.

Everything should be implemented as weak functions that libomptarget should override if it is present. Did I miss one?

After this patch, omp_get_num_devices() seems to wrongly return 0 even when devices are present.

Can you enable LIBOMPTARGET_DEBUG and see if this is printed from libomptarget to confirm libomptarget is loaded and its api is used
DP("Call to omp_get_num_devices returning %zd\n", Devices_size);

After this patch, omp_get_num_devices() seems to wrongly return 0 even when devices are present.

OK, on my internal build these are showing up as weak symbols with nm. But on my LLVM build, not. Will look into the LLVM build to make sure it allows for weak attribute.

After this patch, omp_get_num_devices() seems to wrongly return 0 even when devices are present.

OK, on my internal build these are showing up as weak symbols with nm. But on my LLVM build, not. Will look into the LLVM build to make sure it allows for weak attribute.

I was wrong -- here are the weak symbols that are showing up in libomp:

nm runtime/exports/lin_32e.50.ompt.optional/lib/libomp.so | grep " W "
00000000000a4540 W omp_get_device_num
00000000000a5dd0 W omp_get_device_num_
00000000000a3f20 W omp_get_initial_device
00000000000a57c0 W omp_get_initial_device_
00000000000a3ee0 W omp_get_num_devices@@VERSION
00000000000a5780 W omp_get_num_devices_@@VERSION
00000000000a3f10 W omp_is_initial_device@@VERSION
00000000000a57b0 W omp_is_initial_device_@@VERSION
00000000000a7d20 W ompt_start_tool

So, it looks like weak symbols work better with static libraries. For dynamic, try linking libomptarget first.

After this patch, omp_get_num_devices() seems to wrongly return 0 even when devices are present.

OK, on my internal build these are showing up as weak symbols with nm. But on my LLVM build, not. Will look into the LLVM build to make sure it allows for weak attribute.

I was wrong -- here are the weak symbols that are showing up in libomp:

nm runtime/exports/lin_32e.50.ompt.optional/lib/libomp.so | grep " W "
00000000000a4540 W omp_get_device_num
00000000000a5dd0 W omp_get_device_num_
00000000000a3f20 W omp_get_initial_device
00000000000a57c0 W omp_get_initial_device_
00000000000a3ee0 W omp_get_num_devices@@VERSION
00000000000a5780 W omp_get_num_devices_@@VERSION
00000000000a3f10 W omp_is_initial_device@@VERSION
00000000000a57b0 W omp_is_initial_device_@@VERSION
00000000000a7d20 W ompt_start_tool

So, it looks like weak symbols work better with static libraries. For dynamic, try linking libomptarget first.

I don't think it is a good idea to produce different results just because of the order of libomp and libomptarget. I consider this as a conflict and incompatibility. It must be fixed.

I've reverted the patch, we will investigate a solution into this issue.