[openmp-target-tests] OpenMP 4.5 Target data test cases

[sergiop]

Summary

This patch contains test cases for offloading in openmp 4.5. Specifically, these test cases cover the 'target data' construct.

test_target_data_if.c
test_target_data_map_array_sections.c
test_target_data_use_device_ptr.c
test_target_data_map.c
test_target_data_map_classes.cpp
test_target_data_map_devices.c

[Hahnfeld]

Hi,

I don't see any new files...? I'm also not entirely sure that we should have Fortran tests? @jlpeyton

The changes to offloading_success.{c,cpp} are already in the tree, please rebase your changes to the latest commit.
Phabricator say "Context not available". Did you generate the patches with the -U option? See http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Regards,
Jonas

[sergiop]

Hi Jonas,

Thank you for the comments. I generated again the patch following your comments.

Best regards,

Sergio

[Hahnfeld]

In D34701#794365, @sergiop wrote:

Thank you for the comments. I generated again the patch following your comments.

Now the patch looks like the changes done by clang-format to already existing tests?

[sergiop]

Jonas,

I squashed all the commits into one. Now, I see that Diff has the creation of the new files.

Let me know if this patch works.

Best

[schandra]

Hi Jonas

Did you have further comments on this commit?

Thanks
Sunita

In D34701#797206, @sergiop wrote:

Jonas,

I squashed all the commits into one. Now, I see that Diff has the creation of the new files.

Let me know if this patch works.

Best

In D34701#797206, @sergiop wrote:

Jonas,

I squashed all the commits into one. Now, I see that Diff has the creation of the new files.

Let me know if this patch works.

Best

In D34701#797206, @sergiop wrote:

Jonas,

I squashed all the commits into one. Now, I see that Diff has the creation of the new files.

Let me know if this patch works.

Best

[Hahnfeld]

In D34701#823166, @schandra wrote:

Hi Jonas

Did you have further comments on this commit?

Thanks
Sunita

Sorry that it's taken so long! Still not sure about the Fortran test as clang will currently not be able to handle this.

Additionally, these tests currently won't pass because omp_is_initial_device() will return 1 when offloading to the host, correct? See D35719 why we had to disable libomptarget for 5.0.

Find some more comments inline. In general, the tests should be brought to a consistent coding style (braces, end-comments, comment style and so on).

libomptarget/test/offloading/target_data/test_target_data_map.c

27 ↗

(On Diff #104925)

Please, add braces here. This is really hard to read and the style must be consistent accross the tests.

(The same applies to all other places in this file where this occurrs)

70–73 ↗

(On Diff #104925)

This does not test to. Did you mean to use += 1?

74 ↗

(On Diff #104925)

s/data target/target data/

203 ↗

(On Diff #104925)

s/cheking/checking/

libomptarget/test/offloading/target_data/test_target_data_map_array_sections.c

383 ↗

(On Diff #104925)

If I remember correctly, this will not force an array of length N. To avoid this misleading information, would you mind changing the signature to int *?

libomptarget/test/offloading/target_data/test_target_data_map_classes.cpp

34 ↗

(On Diff #104925)

Do we need an object on the heap, does this also work with the stack? If it should, please add another test method to check that.

42–43 ↗

(On Diff #104925)

I don't really understand the purpose of this test: Wouldn't it be more intuitive if the user hadn't to worry about how the array got there? What would happen if mapping obj->h_array directly, would compiler and runtime perform this work?

libomptarget/test/offloading/target_data/test_target_data_map_devices.c

25–26 ↗

(On Diff #104925)

I know that this variable allocation on the stack works, but could you change this to a malloc to avoid this rather ugly part of the C language?

130 ↗

(On Diff #104925)

Does this really work with libomptarget? If so, is this mandated by the standard and do we want to test this?

libomptarget/test/offloading/target_data/test_target_data_use_device_ptr.c

30 ↗

(On Diff #104925)

array_device[i]?

[schandra]

In D34701#828682, @Hahnfeld wrote:

In D34701#823166, @schandra wrote:

Hi Jonas

Did you have further comments on this commit?

Thanks
Sunita

Sorry that it's taken so long! Still not sure about the Fortran test as clang will currently not be able to handle this.

Additionally, these tests currently won't pass because omp_is_initial_device() will return 1 when offloading to the host, correct? See D35719 why we had to disable libomptarget for 5.0.

Find some more comments inline. In general, the tests should be brought to a consistent coding style (braces, end-comments, comment style and so on).

Great!! Thanks Jonas, we will fix these shortly.
Sunita

[sergiop]

Hi Jonas,

We addressed your comments about the code + removed the fortran file (you're right, sorry about that). I'm submitting an update to the patch.

Thank you for the time. Best

libomptarget/test/offloading/target_data/test_target_data_map_classes.cpp

42–43 ↗

(On Diff #104925)

Unfortunately, the mapping is a shallow copy of the object. This means that the address of the array is not translated automatically.

libomptarget/test/offloading/target_data/test_target_data_map_devices.c

130 ↗

(On Diff #104925)

In the spec there is not restriction about assigning computation/data movement to a device that has an id higher than the number of actual devices.

[sergiop]

Updated the patch with respect to the comments. Special focus on: 1) not using omp_is_initial_device as a condition to fail or pass, 2) removing fortran files, 3) consistent style.

test_target_data_if.c
test_target_data_map_array_sections.c
test_target_data_use_device_ptr.c
test_target_data_map.c
test_target_data_map_classes.cpp
test_target_data_map_devices.c

[Hahnfeld]

1. Is there as specific reason for using both puts and printf? There are also calls to printf without formatting arguments...
2. Do these tests actually pass with a current version of clang?

libomptarget/test/offloading/target_data/test_target_data_if.c

38–40 ↗

(On Diff #110892)

Again thinking about this, the check may be wrong: The OpenMP standard says that the target data and target constructs execute on the host, if the if clause evaluates to false. This check is assuming that it doesn't execute at all, right?

libomptarget/test/offloading/target_data/test_target_data_map_array_sections.c

428 ↗

(On Diff #110892)

Please change this signature as well.

435 ↗

(On Diff #110892)

Same

libomptarget/test/offloading/target_data/test_target_data_map_devices.c

130 ↗

(On Diff #104925)

"The device expression must evaluate to a non-negative integer value less than the value of omp_get_num_devices()."
OpenMP 4.5, p. 97

[AndreyChurbanov]

libomptarget/test/offloading/target_data/test_target_data_if.c

38–40 ↗

(On Diff #110892)

This check looks like a dead code. The size gets three values - 256 to execute on host, 512 and 758 to execute on device. Neither of these values is less than 256, so (size < 256) is the same as false.

Maybe the intention was to check all 1024 values, and zeros should hold for (i >= size).

libomptarget/test/offloading/offloading_success.c

 int main(void) {
   int isHost = -1;
 
-#pragma omp target
+#pragma omp target map(from:isHost)
   { isHost = omp_is_initial_device(); }
 
   if (isHost < 0) {

libomptarget/test/offloading/offloading_success.cpp

 int main(void) {
   int isHost = 0;
 
-#pragma omp target
+#pragma omp target map(from: isHost)
   { isHost = omp_is_initial_device(); }
 
   if (isHost < 0) {