[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:204
@@ +203,3 @@
+  /// Type of functions used as host platform kernels.
+  using HostFunctionTy = std::function<void(const void *const *)>;
+
----------------
jlebar wrote:
> Is the second const necessary?  I feel like it kind of confuses things without really affecting the meaning of the signature in any way.  And the second const has no bearing on whether or not you can bind an std::function to a given function: https://godbolt.org/g/XiksZ9
You were right, it wasn't necessary. I removed it.

================
Comment at: streamexecutor/include/streamexecutor/KernelSpec.h:223
@@ -218,2 +222,3 @@
   }
+  bool hasHostFunction() const { return static_cast<bool>(HostFunction); }
 
----------------
jlebar wrote:
> Nit, maybe `HostFunction != nullptr` would be clearer?  They mean the same thing.
It's a unique_ptr now and it uses the `!= nullptr` test.

================
Comment at: streamexecutor/include/streamexecutor/KernelSpec.h:242
@@ -236,1 +241,3 @@
 
+  const HostFunctionTy *getHostFunction() const {
+    assert(hasHostFunction() && "getting spec that is not present");
----------------
jlebar wrote:
> Why do we return this as a pointer rather than a reference?
Changed it to return a reference.

================
Comment at: streamexecutor/include/streamexecutor/platforms/host/HostPlatform.h:37
@@ +36,3 @@
+    }
+    return new Device(new HostPlatformDevice());
+  }
----------------
jlebar wrote:
> Isn't the Platform supposed to own the Device object?
Right. I totally forgot about that. Now it does.

================
Comment at: streamexecutor/include/streamexecutor/platforms/host/HostPlatformDevice.h:38
@@ +37,3 @@
+    }
+    return static_cast<const void *>(Spec.getHostFunction());
+  }
----------------
jlebar wrote:
> Oh, I see why you return a pointer from getHostFunction().
> 
> Um.  You could still return a reference and take the address here...  That doesn't actually seem so bad to me.
> 
> In either case you probably want to make these multi-kernel objects not-movable, or otherwise allocate the std::function inside a unique_ptr.  Because if this thing gets moved we're currently screwed.
Now returning a ref and taking the address.

Also converted the `MultiKernelLoaderSpec` to store a unique_ptr to the function in order to correctly handle moving.

================
Comment at: streamexecutor/include/streamexecutor/platforms/host/HostPlatformDevice.h:151
@@ +150,2 @@
+
+#endif // STREAMEXECUTOR_PLATFORMS_HOST_HOSTPLATFORMDEVICE_H
----------------
jlebar wrote:
> Does this file supercede the for-testing platform we have?  (Not necessary to do the switch in this patch, of course.)
Yes, I think I will get rid of the for-testing host platform in another patch.


https://reviews.llvm.org/D24473