[Openmp-commits] [PATCH] D154312: [Libomptarget] Begin implementing support for RPC services

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

LG, minor nits



================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.h:258
   virtual Error launchImpl(GenericDeviceTy &GenericDevice, uint32_t NumThreads,
-                           uint64_t NumBlocks,
-                           KernelArgsTy &KernelArgs, void *Args,
+                           uint64_t NumBlocks, KernelArgsTy &KernelArgs,
+                           void *Args,
----------------
jdoerfert wrote:
> jdoerfert wrote:
> > jhuber6 wrote:
> > > tianshilei1992 wrote:
> > > > looks like irrelevant changes
> > > People somehow commit changes without clang-formatting and then it ends up here because my editor will auto format. Somewhat annoying because I've clang formatted all of these files at least twice by now. I'll trim it.
> > Set your editor to only format the lines you changed. Or clang format and precommit.
> recommit
unrelated. precommit


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/RPC.cpp:50
+  return false;
+#endif
+}
----------------
I'd again swap the cases, also below.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/RPC.cpp:136
+
+  return Handles[Device.getDeviceId()].get();
+#else
----------------
Hoist getDeviceId, 4 times makes it hard to read.


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/RPC.cpp:139
+  return plugin::Plugin::error(
+      "Attempt to get an RPC device while not availible");
+#endif
----------------
Check spelling


================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/RPC.h:44
+    llvm::Error deinitDevice() { return Server.deinitDevice(Device); }
+
+    RPCServerTy &Server;
----------------
private:


================
Comment at: openmp/libomptarget/plugins-nextgen/cuda/src/rtl.cpp:482
+      Res = cuStreamSynchronize(Stream);
+    }
 
----------------
Swap the cases, simple first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154312