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

[Erich Focht via Phabricator via Openmp-commits]

efocht added a comment.

Thanks for pushing this upstream, Manoel! Some comments below, inline.



================
Comment at: openmp/libomptarget/plugins/ve/src/rtl.cpp:50
+#define VEO_MIN_VERSION 4
+#define VEO_MAX_VERSION 7
+
----------------
Please get rid of the VEO_MAX_VERSION. The VEO calls that you use are stable and will not be changed, so you won't need this. Also you'd want to build libomptarget with AVEO because it is faster and less limited. I plan to change AVEOs API version number because there are new calls inside, but have no agreement with the VEOS devs whether it shall become "8" or get an offset to VEO API versions.


================
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;
----------------
simoll wrote:
> Can we assume that?
No. Devices can be missing or offline. For sizing the objects below this is ok, but not for selecting the ve nodeids.


================
Comment at: openmp/libomptarget/plugins/ve/src/rtl.cpp:189
+// target RTL.
+int32_t __tgt_rtl_number_of_devices(void) { return DeviceInfo.NumDevices; }
+
----------------
I think you need the online devices here, so you'll need to keep the node_info structure around and check the status array, while going through the nodeids.


================
Comment at: openmp/libomptarget/plugins/ve/src/rtl.cpp:219
+    return OFFLOAD_FAIL;
+  }
+  DP("Available VEO version: %i (supported)\n", veo_version);
----------------
See above: would be good to delete this else block because VEO_MAX_VERSION is not actually needed.


================
Comment at: openmp/libomptarget/plugins/ve/src/rtl.cpp:445
+  for (int32_t i = 0; i < NumArgs; ++i) {
+    veo_args_set_u64(TargetArgs, i, (intptr_t)Args[i]);
+  }
----------------
Please at least accumulate the return codes as the call actually can fail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76843