[Parallel_libs-commits] [PATCH] D24473: [SE] Host platform implementation

[Jason Henline via Parallel_libs-commits]

jhen added inline comments.

================
Comment at: streamexecutor/include/streamexecutor/KernelSpec.h:273
@@ +272,3 @@
+                                         HostFunctionTy Function) {
+    HostFunction = llvm::make_unique<HostFunctionTy>(Function);
+    return *this;
----------------
jlebar wrote:
> Nit, `std::move(Function)`.  (std::function objects may be heavyweight in general, even though in this case it probably doesn't matter.)
I made that change and also changed the signature of the calling function, `addHostFunction` to take the function argument as an rvalue ref to prevent the copy during the function call.

================
Comment at: streamexecutor/include/streamexecutor/platforms/host/HostPlatform.h:39
@@ +38,3 @@
+      TheDevice = llvm::make_unique<Device>(new HostPlatformDevice());
+    return TheDevice.get();
+  }
----------------
jlebar wrote:
> jlebar wrote:
> > Do we need this to be thread-safe?
> I'm confused where we call delete on the HostPlatformDevice.  It's not super important, but I think if we don't it may show up as a leak in asan, and that means it will show up as a leak in user programs.
Yes, we do want thread safety. Thanks for catching that. I added a mutex to protect it. I didn't do lazy static initialization because, as jprice suggested before, I think we are going to want to add another method to clear out the platform.

I also made the `HostDevicePlatform` explicitly owned by `HostPlatform` so it's clear when it is destroyed.


https://reviews.llvm.org/D24473