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

[Lingda Li via Phabricator via Openmp-commits]

lildmh added inline comments.


================
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.
----------------
JonChesterfield wrote:
> What makes the mapper valid? I don't see any checking in the source. Perhaps just strike the word valid from the comment
If there is a valid pointer, which is generated by compiler, I say it's valid. I will remove 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");
----------------
JonChesterfield wrote:
> What limitation? Why 0xffff?
Because the parent idx in map type has 16 bits, we cannot handle components more than that.


================
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) {
----------------
JonChesterfield wrote:
> Size probably returns size_t, why is the induction variable signed?
I think the indices are always type `int32_t` in libomptarget, so I followed the rules. Otherwise there will be a signed and unsigned compariion warning


================
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",
----------------
JonChesterfield wrote:
> And another copy and paste
Okay, will get the common part into a function


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

https://reviews.llvm.org/D68100