[StreamExecutor] Add Stream::blockHostUntilDone

[jhen]

Summary

Add the type-safe wrapper to the platform-specific implementation.

[jlebar]

I initially thought this should be "thenable", but there's no reason for that. This is good.

streamexecutor/include/streamexecutor/Stream.h

87 ↗

(On Diff #69783)

Would "thenBlockHostUntilDone" be more consistent with the other names? Or are you intentionally being inconsistent to signal that this is a different sort of thing (e.g. it doesn't return a Stream&)?

[jhen]

streamexecutor/include/streamexecutor/Stream.h

87 ↗

(On Diff #69783)

Right, I'm being intentionally inconsistent for just the reason you gave.

Side note: things are kind of funny now because there is no Stream::init method. It used to be S->init().thenA().thenB().block(), now it's S->thenA()->thenB().block(). I almost want to change the names so it's S->aThen().bThen().block(), but of course that doesn't read well because the parameter list is after the "then".

[jlebar]

streamexecutor/include/streamexecutor/Stream.h

87 ↗

(On Diff #69783)

I was wondering about that...

Of course even before you could do something like

Stream *s = ...;
s->init()->thenA().thenB().block();
s->thenC().thenD().block();

Right? That is, we only init the stream once. But I guess that's a less-common use-case?

Should we encourage people to treat streams as ephemeral?

Device.getStream()->thenA().thenB().thenC();

That doesn't scan so bad.

While we're thinking about it, it's also weird that you have to switch between . and ->. Although I'm not sure that returning a Stream* would be an improvement. Maybe...

[jhen]

streamexecutor/include/streamexecutor/Stream.h

87 ↗

(On Diff #69783)

You're right, it was a problem even with the init function because the stream could be reused.

It's probably not right to suggest that Stream references shouldn't be held. The most common case is probably to create one Stream and then use it for everything in your program, and there is also overhead in creating and destroying streams.

We could be silly and make an extra "doer" object that would just be there to make things read like English:

Doer Stream::listenUp();
Doer &Doer::thenA();
Doer &Doer::thenB();
...
Stream *s = ...;
s->listenUp().thenA().thenB();
s->block();

:) We could make all Doer instances ephemeral. I better stop talking about this, I think I'm actually talking myself into this joke suggestion.

I'm also not thrilled with the switch from -> to ., but I think I'd rather live with it than use all ->.

parallel-libs/trunk/streamexecutor/include/streamexecutor/Stream.h

public:
   /// Returns the status created by the first error that occurred while
   /// entraining work on this stream.
   Error getStatus() const {
     llvm::sys::ScopedReader ReaderLock(ErrorMessageMutex);
     if (ErrorMessage)
       return make_error(*ErrorMessage);
     else
       return Error::success();
-  };
+  }
+
+  // Blocks the calling host thread until all work enqueued on this Stream
+  // completes.
+  //
+  // Returns the result of getStatus() after the Stream work completes.
+  Error blockHostUntilDone() {
+    setError(PDevice->blockHostUntilDone(ThePlatformStream.get()));
+    return getStatus();
+  }
 
   /// Entrains onto the stream of operations a kernel launch with the given
   /// arguments.
   ///
   /// These arguments can be device memory types like GlobalDeviceMemory<T> and
   /// SharedDeviceMemory<T>, or they can be primitive types such as int. The
   /// allowable argument types are determined by the template parameters to the
   /// Kernel argument.