This is an archive of the discontinued LLVM Phabricator instance.

[StreamExecutor] Add basic Stream operations
ClosedPublic

Authored by jhen on Aug 9 2016, 1:54 PM.

Details

Summary

Add the Stream class and a few of the operations it supports.

Diff Detail

Repository
rL LLVM

Event Timeline

jhen updated this revision to Diff 67413.Aug 9 2016, 1:54 PM
jhen retitled this revision from to [StreamExecutor] Add basic Stream operations.
jhen updated this object.
jhen added reviewers: jlebar, tra.
jhen added subscribers: parallel_libs-commits, jprice.
jlebar added inline comments.Aug 9 2016, 4:19 PM
streamexecutor/include/streamexecutor/StreamExecutor.h
57 ↗(On Diff #67413)

It looks like Stream is concrete and movable, so can we get rid of the unique_ptr?

streamexecutor/lib/Stream.cpp
36 ↗(On Diff #67413)

So...it's illegal to move a stream if there are any outstanding operations? (Among other reasons, we're not taking a mutex here.) Maybe we should have a comment.

38 ↗(On Diff #67413)

Is it important for us to have an API that would let consumers capture this error, if they wanted to?

jlebar added inline comments.Aug 9 2016, 4:19 PM
streamexecutor/include/streamexecutor/Stream.h
17 ↗(On Diff #67413)

Extra space

21 ↗(On Diff #67413)

Missing a comma before "and".

25 ↗(On Diff #67413)

, and

26 ↗(On Diff #67413)

to be

27 ↗(On Diff #67413)

Perhaps: Multiple Stream instances can be created for the same StreamExecutor. This allows ...

72 ↗(On Diff #67413)

This is intriguing...

Is the idea that you don't want to make SE a friend of Stream, because that's too much of a layering violation?

I wonder if you could make SEKey a (private?) type inside Stream.

109 ↗(On Diff #67413)

LLVM style is to leave off curly braces where possible.

113 ↗(On Diff #67413)

Maybe s/ for the invocation//?

122 ↗(On Diff #67413)

Do you want Params&&... and std::forward(Arguments...) below?

124 ↗(On Diff #67413)

Do we want std::move(ArgumentArray)? (At the moment it doesn't make sense because thenRawLaunch takes a const ref to the pack, but see below...)

132 ↗(On Diff #67413)

StreamExecutor::registerHostMemory might be a better name, but we can cross that bridge.

136 ↗(On Diff #67413)

These default args seem weird...would this be better as two functions, one that does a full copy and one that takes an ElementCount?

203 ↗(On Diff #67413)

"up to this point"?

216 ↗(On Diff #67413)

Could we store an Error internally, instead of a string?

jhen updated this revision to Diff 67445.Aug 9 2016, 5:57 PM
jhen marked 13 inline comments as done.
  • Respond to jlebar's comments
streamexecutor/include/streamexecutor/Stream.h
72 ↗(On Diff #67413)

Is the idea that you don't want to make SE a friend of Stream, because that's too much of a layering violation?

Yes, I thought it would be nice to clarify the interface between StreamExecutor and Stream. Maybe this is overkill, but I thought it seemed lightweight enough to be OK.

I wonder if you could make SEKey a (private?) type inside Stream.

I put SEKey into Stream, but I don't know if there is any way to make it a private type. I think StreamExecutor has to be able to see the definition so it can create an instance of SEKey, but maybe I'm missing something.

122 ↗(On Diff #67413)

Ugh, thanks for catching that ugly copy. I changed the arguments to const Params &... Arguments instead of Params &&... for the following horrifying reason:

When I tried to use Params &&..., I got a compiler error when I tried to call thenLaunch with one int argument and one GlobalDeviceMemory<int> argument. The error said the compiler could not infer the types for Params because the TypedKernel<Params...> argument made it infer int, GlobalDeviceMemory<int> and the Params &&... argument made it infer int, GlobalDeviceMemory<int> &.

So, the && seems to make the compiler infer a reference type for some arguments and that conflicts with the types from TypedKernel<Params...>.

Actually, I think const ref is the right signature for this function anyway, so I don't think we need to figure out a solution for the conflicting type inference. What do you think?

124 ↗(On Diff #67413)

Maybe I missed it, but I didn't see the comment below about thenRawLaunch. I didn't change anything for now until we clear up the confusion.

136 ↗(On Diff #67413)

Thanks for the idea. I was struggling with that design. Multiple functions is much nicer.

216 ↗(On Diff #67413)

The problem I had with trying to use an Error is that the method to get out the error string destroys the error object. This is a side-effect of the way LLVM Error enforces the law that users cannot ignore their errors. So it looked like I was going to have to keep destroying and rebuilding the saved Error every time somebody queried it.

streamexecutor/include/streamexecutor/StreamExecutor.h
57 ↗(On Diff #67413)

Oops, I was experimenting with moving streams and forgot to delete the stuff. Streams shouldn't be movable, so we'll stick with a pointer here.

streamexecutor/lib/Stream.cpp
36 ↗(On Diff #67413)

Sorry I forgot to delete this garbage before. It's gone now and Streams are no longer movable.

38 ↗(On Diff #67413)

I think we can do that in a sane way. I change StreamExecutor::deallocateStream to return void and added a comment that says it will record an error if it fails. I left it as a TODO for now to design the interface for fetching stream destruction errors.

I think I understand the motivation for restricting the use of StreamExecutor::launch/memcpy etc to only the Stream class via the StreamKey struct, but I'm not sure I see the need for the StreamExecutorKey struct. It seems like this is currently just protecting the Stream constructor, but is it really a problem if someone can write new Stream(Executor, nullptr) as well as Executor->createStream()? Don't have strong opinions about this, just curious.

The StreamExecutor class seems to be missing a virtual destructor (warned by Clang).

jhen updated this revision to Diff 67540.Aug 10 2016, 9:44 AM
  • Respond to jprice's comments
jhen added a comment.Aug 10 2016, 9:50 AM

I think I understand the motivation for restricting the use of StreamExecutor::launch/memcpy etc to only the Stream class via the StreamKey struct, but I'm not sure I see the need for the StreamExecutorKey struct. It seems like this is currently just protecting the Stream constructor, but is it really a problem if someone can write new Stream(Executor, nullptr) as well as Executor->createStream()? Don't have strong opinions about this, just curious.

My goal was to make sure that Streams always have non-null implementation pointers. Basically, I didn't want any half-formed Stream objects floating around. This goal was really undercut when I originally left in the code to allow Streams to be moved. After being moved from, a Stream would be half-formed. But now I've removed the move stuff for Streams, and I think that leaves us with a useful invariant that Streams will always be fully-formed.

I added a comment to the Stream constructor to try to clarify this because I think you're right that it's a bit confusing, and I think others will have the same question.

The StreamExecutor class seems to be missing a virtual destructor (warned by Clang).

Thanks for catching that. I have added in a virtual destructor now.

jlebar added inline comments.Aug 10 2016, 10:29 AM
streamexecutor/include/streamexecutor/Stream.h
90 ↗(On Diff #67540)

If that's the only point, could we just assert that Implementation is not null?

One advantage of getting rid of SEKey would be, that (maybe?) we could move StreamKey into Stream itself. Right now we can't do that because you can't forward-declare a nested type, and so there's no way to have StreamKey inside Stream and SEKey inside SE and have them both depend on each other via #includes.

123 ↗(On Diff #67540)

Discussed IRL, it sounds like the point is to pass references, not ownership. So this is good.

125 ↗(On Diff #67540)

Sorry, I meant to add another comment. But it's moot if we're taking const refs.

217 ↗(On Diff #67540)

Hm. Okay, I don't see a better way to do this at the moment. If we had a more complex error class it would be worth worrying about, but as-is, it's fine.

272 ↗(On Diff #67540)

Nit, I think you want llvm::optional<std::string>.

jhen updated this revision to Diff 67572.Aug 10 2016, 12:02 PM
jhen marked 3 inline comments as done.
  • Respond to jlebar's comments (take 2)
streamexecutor/include/streamexecutor/Stream.h
90 ↗(On Diff #67540)

Thinking more about this, I realized that the more important point is that I don't want users dealing with the StreamInterface class at all. It should be private to StreamExecutor. So I think it is good to mark this interface as only callable by StreamExecutor so user's don't get the wrong idea about StreamInterface. I updated the constructor comment to say this instead of talking about null pointers.

I agree that I don't like the way the StreamKey is part of the StreamExecutor class and vice versa. To deal with this, I factored the whole key object pattern out into a header called Passkey<T> that makes it possible for any class to make its own key instance. I think this cleans up the design significantly.

jlebar added inline comments.Aug 10 2016, 1:45 PM
streamexecutor/include/streamexecutor/Passkey.h
1 ↗(On Diff #67572)

Move into Util?

41 ↗(On Diff #67572)

Should we take Passkey<B> by value instead of by reference?

55 ↗(On Diff #67572)

We call it a "reference" in the comment and here, but apparently it's a pointer; that should be harmonized. I kind of like the idea of keeping a proper C++ reference, so it's clear that it's never null.

62 ↗(On Diff #67572)

Not sure about the "&". In fact I would expect that if you have

void foo(Passkey<Bar>& b);

and then have

void Bar::DoSomething() {
  foo(this);
}

that won't compile because we're trying to bind a temporary Passkey to b.

65 ↗(On Diff #67572)

I think you'll need a copy-constructor if you want to pass these by value. I guess we could take them by rvalue reference if you really wanted to disallow copying. But even then we'd still need to allow moves, which is almost as bad from an access-control perspective.

So I'm not sure disallowing copying is worthwhile.

streamexecutor/include/streamexecutor/Stream.h
91 ↗(On Diff #67572)

I realized that the more important point is that I don't want users dealing with the StreamInterface class at all. It should be private to StreamExecutor.

Can't SE simply avoid passing out references to StreamInterfaces, then?

For the "private-to-Stream" functions in SE, I'm wondering something similar: It seems like maybe SE should be split into two types. One type is visible to end-users, and it contains only functions they'd call. The other type is visible only to e.g. Stream, and it actually does work. Stream would take a pointer to that thing in its constructor.

Since SE isn't going to give out pointers to this "internal SE" except when creating Streams, we get access control for free.

jhen updated this revision to Diff 67620.Aug 10 2016, 3:36 PM
jhen marked 5 inline comments as done.
  • Respond to jlebar's comments (take 3)
streamexecutor/include/streamexecutor/Passkey.h
1 ↗(On Diff #67572)

I got rid of this class (see comment below), so I'm marking all the comments on this class as "done". Thanks for suggesting a better way to handle all this.

streamexecutor/include/streamexecutor/Stream.h
91 ↗(On Diff #67572)

Yes, I think the design you outline here is much better. I created a class called StreamParent to hold the interface that a Stream uses to call its parent StreamExecutor. I got rid of the Passkey class and all the key arguments.

This seems much cleaner. Thanks for the advice!

jhen updated this revision to Diff 67638.Aug 10 2016, 5:26 PM
  • Fixes to breaks from last patch
jhen added a comment.Aug 10 2016, 5:27 PM

Previous patch accidentally left things in a broken state. Should be correct now.

jlebar added inline comments.Aug 11 2016, 4:57 PM
streamexecutor/include/streamexecutor/Stream.h
64 ↗(On Diff #67638)

explicit?

110 ↗(On Diff #67638)

LLVM style is to leave off curly braces where possible.

149 ↗(On Diff #67638)

Leave off curly braces? (I know, it's awful, but consistency is good too...) Same below.

streamexecutor/lib/Stream.cpp
51 ↗(On Diff #67638)

I wonder if we need any of these *raw* functions anymore, now that they're just forwarding to the parent.

jhen updated this revision to Diff 67791.Aug 11 2016, 7:02 PM
jhen marked 4 inline comments as done.
  • Remove StreamParent class
jhen added a comment.Aug 11 2016, 7:02 PM

This version gets rid of the StreamParent because it was obscuring the logic about which functions are overridden by each platform. I renamed the file Interfaces.h to PlatformInterfaces.h to try to clarify its purpose as the interfaces that each platform must implement. I also added the PlatformStreamExecutor interface class in PlatformInterfaces.h, and that class takes the place of what StreamParent was trying to do.

Thanks to jlebar for talking this over with me offline to try to get the design right.

streamexecutor/include/streamexecutor/Stream.h
64 ↗(On Diff #67638)

It wasn't meant to be and it's not anymore.

streamexecutor/lib/Stream.cpp
51 ↗(On Diff #67638)

I got rid of them for now, but I might bring them back later if users need to include Stream.h and I don't want them to see the PlatformInterface.h file.

FWIW, this design feels a lot cleaner to me :-)

streamexecutor/include/streamexecutor/PlatformInterfaces.h
48 ↗(On Diff #67791)

Missing virtual destructor in this class.

60 ↗(On Diff #67791)

Should there be a corresponding platform-specific allocateStream()?

jhen updated this revision to Diff 67859.Aug 12 2016, 10:45 AM
jhen marked an inline comment as done.
  • Add PlatformStream
streamexecutor/include/streamexecutor/PlatformInterfaces.h
60 ↗(On Diff #67791)

Really good point. In response, I changed this part of the design. Now there is a PlatformStream base class that is responsible for cleaning up its resources during destruction, so there is no more deallocateStream method.

I also changed the methods in PlatformStreamExecutor to accept PlatformStream pointers rather than Stream pointers. This seems to fit better with the design of having the Platform class methods take raw handles rather than public wrappers.

One last nit from me.

streamexecutor/include/streamexecutor/Stream.h
51 ↗(On Diff #67859)

This declaration isn't needed anymore.

jhen updated this revision to Diff 67871.Aug 12 2016, 11:36 AM
  • Remove StreamParent forward decl
jhen marked an inline comment as done.Aug 12 2016, 11:36 AM
jhen added inline comments.
streamexecutor/include/streamexecutor/Stream.h
52 ↗(On Diff #67871)

Thanks for catching that.

jlebar edited edge metadata.Aug 12 2016, 12:30 PM

Sorry to keep nit'ing on the design here. I think each revision has been an improvement, though; hopefully we don't start going in circles.

streamexecutor/include/streamexecutor/PlatformInterfaces.h
42 ↗(On Diff #67871)

If Stream is still calling into PlatformSE to do its work (so this class is just an RAII thing), maybe PlatformStreamHandle would be a better name?

streamexecutor/include/streamexecutor/Stream.h
64 ↗(On Diff #67871)

Now we're kind of taking redundant information here. PStream's PE must match PlatformExecutor, right?

It doesn't seem so bad to me if PStream exposed a pointer back to its PExecutor...

streamexecutor/include/streamexecutor/StreamExecutor.h
30 ↗(On Diff #67871)

What does it mean to have an SE without a backing PlatformSE?

31 ↗(On Diff #67871)

Presumably you want explicit?

jhen updated this revision to Diff 67890.Aug 12 2016, 1:17 PM
jhen marked 5 inline comments as done.
jhen edited edge metadata.
  • Tweaks to PlatformStream
streamexecutor/include/streamexecutor/PlatformInterfaces.h
42 ↗(On Diff #67871)

Yes, that is a better name. I made the change.

streamexecutor/include/streamexecutor/StreamExecutor.h
30 ↗(On Diff #67871)

I just forgot to delete this ctor during development. It is gone now.

jhen added a comment.Aug 12 2016, 1:21 PM

Sorry to keep nit'ing on the design here. I think each revision has been an improvement, though; hopefully we don't start going in circles.

No worries from my end. I definitely feel like things are getting better each time. Thanks for your help!

jlebar added inline comments.Aug 12 2016, 1:24 PM
streamexecutor/include/streamexecutor/PlatformInterfaces.h
45 ↗(On Diff #67890)

Why do we allow a null executor?

57 ↗(On Diff #67890)

Not sold on the "parent" part of the name. At best it seems redundant. Would it be confusing if we just called it "executor"?

streamexecutor/lib/StreamExecutor.cpp
35 ↗(On Diff #67890)

Hm, I think this should be the PE's responsibility, since otherwise anyone who calls PE::createStream will have to remember to do this. We can assert it here if you want.

jhen updated this revision to Diff 67909.Aug 12 2016, 1:57 PM
jhen marked 3 inline comments as done.
  • Make platform exec set stream exec during creation
streamexecutor/include/streamexecutor/PlatformInterfaces.h
45 ↗(On Diff #67890)

This was to allow a default construction followed by a call to setParentExecutor. It's removed now because it is not needed since I moved the responsibility of setting the executor pointer to the platform-specific implementation (as per jlebar's comment below).

jlebar accepted this revision.Aug 12 2016, 1:59 PM
jlebar edited edge metadata.
This revision is now accepted and ready to land.Aug 12 2016, 1:59 PM
This revision was automatically updated to reflect the committed changes.
parallel-libs/trunk/streamexecutor/CMakeLists.txt