[Openmp-commits] [PATCH] D68100: [OpenMP 5.0] declare mapper runtime implementation

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield requested changes to this revision.
JonChesterfield added a comment.
This revision now requires changes to proceed.

The premise seems OK, but three copies of a large block of control flow is not so good. Why the duplication?



================
Comment at: openmp/libomptarget/src/omptarget.cpp:349
 
-    // Helper function to get the base address and type.
-    auto &&GetBegin = [&args](int32_t Idx) { return args[Idx]; };
-    auto &&GetType = [&arg_types](int32_t Idx) { return arg_types[Idx]; };
-    int rt = target_data_begin_component(Device, args[i], arg_sizes[i],
-                                         arg_types[i], &args_base[i], i,
-                                         arg_num, GetBegin, GetType);
-    if (rt != OFFLOAD_SUCCESS)
-      return OFFLOAD_FAIL;
+    // If a valid user-defined mapper is attached, use the associated mapper
+    // function to complete data mapping.
----------------
What makes the mapper valid? I don't see any checking in the source. Perhaps just strike the word valid from the comment


================
Comment at: openmp/libomptarget/src/omptarget.cpp:359
+                       arg_types[i]);
+      if (Components.size() >= 0xffff) {
+        DP("The number of components exceed the limitation\n");
----------------
What limitation? Why 0xffff?


================
Comment at: openmp/libomptarget/src/omptarget.cpp:366
+      for (int32_t j = 0, e = Components.size(); j < e; ++j) {
+        // Helper function to get the base address and type.
+        auto &&GetBegin = [&Components](int32_t Idx) {
----------------
Size probably returns size_t, why is the induction variable signed?


================
Comment at: openmp/libomptarget/src/omptarget.cpp:536
+      MapperComponentsTy Components;
+      MapperFuncPtrTy MapperFuncPtr = (MapperFuncPtrTy)(arg_mappers[i]);
+      (*MapperFuncPtr)((void *)&Components, args_base[i], args[i], arg_sizes[i],
----------------
This appears to be a copy and paste of the above


================
Comment at: openmp/libomptarget/src/omptarget.cpp:667
+    // function to complete data mapping.
+    if (arg_mappers && arg_mappers[i]) {
+      DP("Call the mapper function " DPxMOD " for the %dth argument\n",
----------------
And another copy and paste


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68100/new/

https://reviews.llvm.org/D68100