[Openmp-commits] [PATCH] D76843: [Openmp] Libomptarget plugin for NEC SX-Aurora

[Simon Moll via Phabricator via Openmp-commits]

simoll added a comment.

Thanks for working on this!
Comments are mostly nitpicks. I am not sure we can assume that the VEOS device ids are consecutive.



================
Comment at: openmp/libomptarget/plugins/ve/src/rtl.cpp:10
+//
+// RTL for NEC Aurora TSUBASE machines
+//
----------------
typo: Aurora TSUBASE


================
Comment at: openmp/libomptarget/plugins/ve/src/rtl.cpp:122
+    // Dynamically check how many devices we have
+    // For the moment we assume that VEO device IDs are consecutive
+    struct ve_nodeinfo node_info;
----------------
Can we assume that?


================
Comment at: openmp/libomptarget/plugins/ve/src/rtl.cpp:168
+    return OFFLOAD_FAIL;
+  } else {
+    DP("Function at address %p called (VEO request ID: %" PRIu64 ")\n",
----------------
else after return


================
Comment at: openmp/libomptarget/plugins/ve/src/rtl.cpp:214
+    return OFFLOAD_FAIL;
+  } else if (veo_version > VEO_MAX_VERSION) {
+    DP("veo_get_version() reported version %i, Maximum supported veo api "
----------------
else after return


================
Comment at: openmp/libomptarget/plugins/ve/src/rtl.cpp:222
+
+  // At the moment we do not really initialize (i.e. creaete a process or
+  // context on) the device here, but in "__tgt_rtl_load_binary".
----------------
typo: craete


================
Comment at: openmp/libomptarget/plugins/ve/src/rtl.cpp:298
+      }
+    } else {
+      proc_handle = veo_proc_create_static(ID, tmp_name);
----------------
else after return


================
Comment at: openmp/libomptarget/plugins/ve/src/rtl.cpp:336
+    DP("Successfully loaded library dynamically\n");
+  } else {
+    DP("Symbol table is expected to have been created by "
----------------
else after return


================
Comment at: openmp/libomptarget/plugins/ve/src/rtl.cpp:362
+void *__tgt_rtl_data_alloc(int32_t ID, int64_t Size, void *HostPtr) {
+  uint64_t ret;
+  uint64_t addr;
----------------
`int` according to https://github.com/veos-sxarr-NEC/veoffload/blob/master/include/ve_offload.h


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76843