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

[Justin Lebar via Parallel_libs-commits]

jlebar 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 *)>;
+
----------------
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

================
Comment at: streamexecutor/include/streamexecutor/KernelSpec.h:223
@@ -218,2 +222,3 @@
   }
+  bool hasHostFunction() const { return static_cast<bool>(HostFunction); }
 
----------------
Nit, maybe `HostFunction != nullptr` would be clearer?  They mean the same thing.

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

================
Comment at: streamexecutor/include/streamexecutor/platforms/host/HostPlatform.h:37
@@ +36,3 @@
+    }
+    return new Device(new HostPlatformDevice());
+  }
----------------
Isn't the Platform supposed to own the Device object?

================
Comment at: streamexecutor/include/streamexecutor/platforms/host/HostPlatformDevice.h:38
@@ +37,3 @@
+    }
+    return static_cast<const void *>(Spec.getHostFunction());
+  }
----------------
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.

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


https://reviews.llvm.org/D24473