This is an archive of the discontinued LLVM Phabricator instance.

[SE] Host platform implementation
ClosedPublic

Authored by jhen on Sep 12 2016, 2:23 PM.

Details

Summary

This implementation does not currently support multiple concurrent streams, and
it won't allow kernels to be launched with grids larger than one block or
blocks larger than one thread. These limitations could be removed in the future
by launching new threads on the host, but that is not done in this
implementation.

Diff Detail

Event Timeline

jhen updated this revision to Diff 71053.Sep 12 2016, 2:23 PM
jhen retitled this revision from to [SE] Host platform implementation.
jhen added a reviewer: jlebar.
jhen updated this object.
jhen added subscribers: parallel_libs-commits, jprice.
jlebar added inline comments.Sep 12 2016, 5:51 PM
streamexecutor/include/streamexecutor/KernelSpec.h
205

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

224

Nit, maybe HostFunction != nullptr would be clearer? They mean the same thing.

243

Why do we return this as a pointer rather than a reference?

streamexecutor/include/streamexecutor/platforms/host/HostPlatform.h
38

Isn't the Platform supposed to own the Device object?

streamexecutor/include/streamexecutor/platforms/host/HostPlatformDevice.h
39

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.

152

Does this file supercede the for-testing platform we have? (Not necessary to do the switch in this patch, of course.)

jhen updated this revision to Diff 71184.Sep 13 2016, 9:11 AM
jhen marked 6 inline comments as done.
  • Respond to jlebar's comments
streamexecutor/include/streamexecutor/KernelSpec.h
205

You were right, it wasn't necessary. I removed it.

224

It's a unique_ptr now and it uses the != nullptr test.

243

Changed it to return a reference.

streamexecutor/include/streamexecutor/platforms/host/HostPlatform.h
38

Right. I totally forgot about that. Now it does.

streamexecutor/include/streamexecutor/platforms/host/HostPlatformDevice.h
39

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.

152

Yes, I think I will get rid of the for-testing host platform in another patch.

jlebar added inline comments.Sep 13 2016, 9:22 AM
streamexecutor/include/streamexecutor/KernelSpec.h
273

Nit, std::move(Function). (std::function objects may be heavyweight in general, even though in this case it probably doesn't matter.)

streamexecutor/include/streamexecutor/platforms/host/HostPlatform.h
40

Do we need this to be thread-safe?

40

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.

jhen updated this revision to Diff 71193.Sep 13 2016, 10:11 AM
jhen marked an inline comment as done.
  • Make thread safe and destroy PlatformDevice
streamexecutor/include/streamexecutor/KernelSpec.h
273

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.

streamexecutor/include/streamexecutor/platforms/host/HostPlatform.h
40

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.

jlebar accepted this revision.Sep 13 2016, 10:18 AM
jlebar edited edge metadata.
jlebar added inline comments.
streamexecutor/include/streamexecutor/KernelSpec.h
273

The usual convention I've seen is to take the heavyweight, copyable arg (be it an std::function, std::string, std::vector, whatever) that you're taking ownership of by value, rather than by rvalue ref.

You're right that making the function arg an rvalue ref prevents accidental copies. The problem is that it also makes *intentional* copies a pain. I think you have to spell out the whole type again:

std::function<const void*> fn = ...;
addHostFunction("kernel_name", std::function<const void*>(fn));
This revision is now accepted and ready to land.Sep 13 2016, 10:18 AM
jhen updated this revision to Diff 71217.Sep 13 2016, 12:23 PM
jhen edited edge metadata.
  • Change rvalue ref arg to value
This revision was automatically updated to reflect the committed changes.