[Openmp-commits] [PATCH] D34701: [openmp-target-tests] OpenMP 4.5 Target data test cases

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added a comment.

In https://reviews.llvm.org/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 https://reviews.llvm.org/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).



================
Comment at: libomptarget/test/offloading/target_data/test_target_data_map.c:27
+#pragma omp target
+    if (!omp_is_initial_device())
+      for (int i = 0; i < N; ++i) {
----------------
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)


================
Comment at: libomptarget/test/offloading/target_data/test_target_data_map.c:70-73
+      for (int i = 0; i < N; ++i) {
+        h_array_h[i] = 1;
+        h_array_s[i] = 1;
+      }
----------------
This does not test `to`. Did you mean to use `+= 1`?


================
Comment at: libomptarget/test/offloading/target_data/test_target_data_map.c:74
+      }
+  } // end of data target region
+
----------------
s/data target/target data/


================
Comment at: libomptarget/test/offloading/target_data/test_target_data_map.c:203
+      }
+      // cheking errors
+      d_sum[0] = 0; // pointer arithmetic is not supported on the devices for
----------------
s/cheking/checking/


================
Comment at: libomptarget/test/offloading/target_data/test_target_data_map_array_sections.c:383
+
+void init_1d(int a[N]) {
+  for (int i = 0; i < N; ++i)
----------------
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 *`?


================
Comment at: libomptarget/test/offloading/target_data/test_target_data_map_classes.cpp:34
+  int *array = new int[N];
+  A *obj = new A(array, N);
+
----------------
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.


================
Comment at: libomptarget/test/offloading/target_data/test_target_data_map_classes.cpp:42-43
+      // assign device array ptr to device obj
+      int *tmp_h_array = obj->h_array;
+      obj->h_array = array;
+      int tmp = 0;
----------------
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?


================
Comment at: libomptarget/test/offloading/target_data/test_target_data_map_devices.c:25-26
+
+  int sum[num_dev], errors = 0;
+  int h_matrix[num_dev][N];
+
----------------
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?


================
Comment at: libomptarget/test/offloading/target_data/test_target_data_map_devices.c:130
+        else if (dev >= real_num_dev) {
+          // allows implementations that map extra devices to the host
+          extra_on_host = 1;
----------------
Does this really work with libomptarget? If so, is this mandated by the standard and do we want to test this?


================
Comment at: libomptarget/test/offloading/target_data/test_target_data_use_device_ptr.c:30
+        for (i = 0; i < len; i++) {
+          *(array_device + i) = i;
+          array_host[i] += *(array_device + i);
----------------
`array_device[i]`?


https://reviews.llvm.org/D34701