This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][Offloading] Fix the issue that omp_get_num_devices returns wrong number of devices
ClosedPublic

Authored by tianshilei1992 on Jan 12 2020, 11:50 AM.

Details

Summary

This patch is to fix issue in the following simple case:

#include <omp.h>
#include <stdio.h>

int main(int argc, char *argv[]) {
  int num = omp_get_num_devices();
  printf("%d\n", num);

  return 0;
}

Currently it returns 0 even devices exist. Since this file doesn't contain any
target region, the host entry is empty so further actions like initialization
will not be proceeded, leading to wrong device number returned by runtime
function call.

Diff Detail

Event Timeline

tianshilei1992 edited the summary of this revision. (Show Details)Jan 12 2020, 11:50 AM
ABataev requested changes to this revision.Jan 12 2020, 12:20 PM

"Revert" is wrong here since one of the tests will definitely fail after this. Prepare a patch with the proper fix.

This revision now requires changes to proceed.Jan 12 2020, 12:20 PM

"Revert" is wrong here since one of the tests will definitely fail after this. Prepare a patch with the proper fix.

Yep. I got your point. Didn't notice what you tried to fix before. Will update correspondingly.

"Revert" is wrong here since one of the tests will definitely fail after this. Prepare a patch with the proper fix.

Yep. I got your point. Didn't notice what you tried to fix before. Will update correspondingly.

Ok, thanks.

tianshilei1992 retitled this revision from [OpenMP][Offloading] Reverted the change that ignores empty target descriptors to fix the issue which the runtime returns wrong device number when no target region. to [OpenMP][Offloading] Fix the issue that omp_get_num_devices returns wrong number of devices.
tianshilei1992 edited the summary of this revision. (Show Details)
This comment was removed by tianshilei1992.
tianshilei1992 updated this revision to Diff 237840.
tianshilei1992 edited the summary of this revision. (Show Details)
protze.joachim requested changes to this revision.Jan 13 2020, 9:30 PM
protze.joachim added a subscriber: protze.joachim.

From OpenMP spec perspective I see no issue in returning 0 for the provided code even if there would be devices available, as long as this is consistent with the remaining execution of the application.

As I understand the modification, the function would not return 0 if afterwards a target region will be offloaded to a device?

This revision now requires changes to proceed.Jan 13 2020, 9:30 PM
tianshilei1992 added a comment.EditedJan 14 2020, 10:54 AM

From OpenMP spec perspective I see no issue in returning 0 for the provided code even if there would be devices available, as long as this is consistent with the remaining execution of the application.

As I understand the modification, the function would not return 0 if afterwards a target region will be offloaded to a device?

Actually, no because of empty PendingCtorsDtors and empty target entries. And what about the following code, what value do you expect to be returned?

#include <omp.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>

int main() {
  const size_t N = 1UL << 30;
  const int ndev = omp_get_num_devices();
  const int host = omp_get_initial_device();

  printf("Found %d devices, host is %d\n", ndev, host);

  uint8_t *v = (uint8_t *)malloc(N);
  uint8_t *u[ndev];

  for (int i = 0; i < ndev; i++) {
    u[i] = (uint8_t *)omp_target_alloc(N, i);
  }

  for (size_t i = 0; i < N; i++) {
    v[i] = 0;
  }

  for (int i = 0; i < ndev; i++) {
    uint8_t *ui = u[i];

#pragma omp target data device(i) map(tofrom : v [0:N])
    {
#pragma omp target data device(i) use_device_ptr(v)
      { printf("Device %d: v = %p, u = %p\n", i, v, ui); }
    }
  }

  for (int i = 0; i < ndev; i++) {
    omp_target_free(u[i], i);
  }
  free(v);
}

Could you provide full diff of your modificaions?

Could you provide full diff of your modificaions?

This is exactly the full diff.

Could you provide full diff of your modificaions?

This is exactly the full diff.

Nope, it does not have context (see this https://www.llvm.org/docs/Phabricator.html). Also, add a test.

Upload full diff information

ABataev added inline comments.Jan 14 2020, 12:00 PM
openmp/libomptarget/src/omptarget.cpp
97–101

Maybe just check if TransTable->HostTable.EntriesBegin == TransTable->HostTable.EntriesEnd? Along with the check if entries have processed already. Plus, I don't think it is correct to exit from the loop here.

tianshilei1992 added inline comments.Jan 14 2020, 5:47 PM
openmp/libomptarget/src/omptarget.cpp
97–101

Maybe we can make it in this way?

if (TransTable->TargetsTable[device_id] != 0) {
  // Library entries have already been processed
  continue;
}

if (TransTable->HostTable.EntriesBegin == TransTable->HostTable.EntriesEnd) {
  // No entries so no need to process
  continue;
}
ABataev added inline comments.Jan 14 2020, 5:54 PM
openmp/libomptarget/src/omptarget.cpp
97–101

Sure, this one is also fine, if it works correctly. Just, maybe, make this check the first.

tianshilei1992 marked 3 inline comments as done.

Need a test

Reused existing test case with just a little tiny modification to test this patch

ABataev added inline comments.Jan 14 2020, 9:37 PM
openmp/libomptarget/test/api/omp_get_num_devices.c
15

Hmm, better to add a new test, I think, rather than modify this one. We need to increase the coverage, not just change it in some way.

protze.joachim resigned from this revision.Jan 15 2020, 10:01 AM

From OpenMP spec perspective I see no issue in returning 0 for the provided code even if there would be devices available, as long as this is consistent with the remaining execution of the application.

As I understand the modification, the function would not return 0 if afterwards a target region will be offloaded to a device?

Actually, no because of empty PendingCtorsDtors and empty target entries.

Ok.

And what about the following code, what value do you expect to be returned?

I would expect as many devices as available for this execution.

I would expect as many devices as available for this execution.

Yeah, it should be. However, w/o this patch, it returns 0.

tianshilei1992 marked an inline comment as done.Jan 15 2020, 3:40 PM
tianshilei1992 added inline comments.
openmp/libomptarget/test/api/omp_get_num_devices.c
15

Would you recommend to add a real new file or add a new function to cover the case?

ABataev added inline comments.Jan 15 2020, 3:45 PM
openmp/libomptarget/test/api/omp_get_num_devices.c
15

It must be a new file, otherwise there won't be an empty image.

tianshilei1992 added inline comments.Jan 15 2020, 3:49 PM
openmp/libomptarget/test/api/omp_get_num_devices.c
15

Oh! That's right. Shame on me. I should have thought of that. :-|

Add a new test case instead of modifying existing one to improve test coverage

ABataev added inline comments.Jan 15 2020, 4:07 PM
openmp/libomptarget/test/api/omp_get_num_devices_with_empty_target.c
25 ↗(On Diff #238385)

No // CHECK: directives.

tianshilei1992 marked an inline comment as done.Jan 15 2020, 4:14 PM
tianshilei1992 added inline comments.
openmp/libomptarget/test/api/omp_get_num_devices_with_empty_target.c
25 ↗(On Diff #238385)

It is at the last line.

ABataev accepted this revision.Jan 15 2020, 4:16 PM

LG.

openmp/libomptarget/test/api/omp_get_num_devices_with_empty_target.c
25 ↗(On Diff #238385)

Missed it.

This revision is now accepted and ready to land.Jan 15 2020, 4:16 PM

Is anything else I need to do to make it merge?

Is anything else I need to do to make it merge?

You need to commit it.

Is anything else I need to do to make it merge?

You need to commit it.

Hmm, seems like I didn't have commit access?

Is anything else I need to do to make it merge?

You need to commit it.

Hmm, seems like I didn't have commit access?

Ok, will commit it for you.

This revision was automatically updated to reflect the committed changes.

Is anything else I need to do to make it merge?

You need to commit it.

Hmm, seems like I didn't have commit access?

Ok, will commit it for you.

Thank you!