Is this true? Like Acxxel exposes a different interface from the standard CUDA runtime library, so maybe it's not a "drop-in" replacement. But maybe it's a "replacement", and maybe we should say it doesn't require libcuda.

Maybe we can say, "Function type used to destroy opaque handles given out by the Platform." or something.

It's not obvious to me what these three mean; maybe they should have comments?

You know, I thought I knew what this meant, but now I'm not sure.

Same for these three.

Maybe, "Gets a pointer to the platform with the given name" or something, so it's clear that there can be multiple platforms.

Can we expand on this a little bit? Maybe, "All operations on a Stream are serialized."

Nit, s/and/so/ or something.

Which function -- the callback, or addCallback()? (I am legitimately unsure.)

Why not just take std::function<void(Stream&, const Status&)> here?

That would give us better error messages, and it seems like it would simplify a lot of the code in the cc file. It's also a potential speedup -- for example, if Callback is a function pointer, creating an std::function from it doesn't necessarily allocate, whereas the implementation below always calls malloc.

Maybe I'm missing something.

Can we be more specific about the allowed types here? It looks like they need to be convertible to something or other. Same elsewhere where we take universal references to "memory things".

Here too it might be helpful to have a bit more verbiage here telling me what this is good for.

This strikes me kind of as a weird name; the stream "records" the event? But I don't have a better suggestion offhand, and if it's consistent with prior art, maybe it's fine.

If we're giving out floating-point numbers anyway, could we give seconds instead? That's a more natural unit.

Should this have a comment? I can't figure out what this is.

Maybe s/the index of//. It's pretty clear from the signature how the number corresponds to a device, whereas as written I'm kind of confused.

llvm/clang's style is to leave off braces here. Are we adopting a different style? I don't really care myself, but you'll probably get grumbles.

I have no idea what this is doing; can we expand upon this some?

You only care about the type of *DeviceSrc.data() and *DeviceDst.data(), right? Maybe we could just use decltype instead of ::value_type, so we'll be compatible with more types.

(If you don't want to type out the decltype expression over and over, I suppose we could even have our own traits class. But that may be overkill.)

Probably worth adding a comment indicating that if DeviceSrc's size is less than ElementCount, we copy only the first ElementCount elems -- I wasn't sure if that would be an error or what.

Is this better than requiring us to go through a span? That would shrink the API a bit, and (I would hope) make it more obvious what happens when ElementCount doesn't match DeviceSrc's size.

Similar comment here.

Do we actually care about operator[] as opposed to the data function? That's surprising if so... Perhaps we should explicitly write down our "concepts" somewhere.

At least let's add a comment to the header indicating what "concept" Container needs to fulfill.

Since the device one doesn't call T::T() but this one does, maybe we should say that.

Could we call this newAsyncHostMem? That would match registerHostMem, and also just be easier to understand, I think.

Hm, requiring the program to be stored in an std::string may be a problem. Could we accept a Span<char> instead?

I am unclear how this kernel-launching stuff relates to the comment at the top of file saying we don't do this... Is this for opencl only?

Not sure this is a good name, because it makes it sound like it's just calling malloc, when in fact it's also registering the memory.

Nit, it seems kind of inconsistent that we declare these out-of-line but the sync versions inline.

Nit, const is an adjective, so "of const elements" or something?

Remind me why we can't do these inside the class definition?

Hm, this is two indirect calls, one to get the function pointer, and the other to call it.

We could make this just one virtual call, but that would be inconsistent with how you do literally everything else here. So I guess this is fine as-is. If it becomes a performance issue, we can address it then.

This needs noexcept? That's...interesting. Maybe deserves a comment.

Oh, this is the conversion that lets us pass it to a <<<>>> launch? But this pointer is not valid on the host? (Never? Or, it's dereferenceable if we are using unified memory?)

If so, or probably also if not, a comment would be helpful here.

Interesting -- I presume you're doing this because that's what std::span does? Does std::span not have a way to slice that doesn't do a bounds check?

Anyway I think this is very likely fine for our use-case.

Do we need "explicit" here? It makes a subtle difference, but unless you're going for it, maybe we just leave it off?

Probably worth saying, "scope, but does not free itself."

T *Memory = new (MaybeMemory.getValue()) T[ElementCount];

Oh, hm. Are you sure this is the right way to undo your allocation above? Like, we don't have to somehow delete the placement new'ed array itself? (I don't see a way to do it on cppreference.)

I changed it to "modern replacement". Unfortunately it does require libcuda and it also requires libcudart right now to access pointers for device variables. We can maybe get rid of the libcudart dependency for clang in the future by including a real drop-in replacement for libcudart, but it's not at that point now.

I removed these and the config structs below because they were too CUDA-specific, and I don't think they were very helpful anyway.

Now explicitly says addCallback to clarify.

It's CUDA's name for the operation, but I don't like it either. It's misleading, so I'm renaming it to "enqueue".

Actually, I only want DeviceSrcTy and DeviceDstTy to be either DeviceMemory<T> or DeviceMemorySpan<T>, neither of which has a data member function, so I think this is doing the right thing.

Function deleted, as suggested below.

We can get rid of this.

Deleted this function.

I changed this to checking data instead, and I added a comment to try to describe what is expected of Container.

It turns out I was using this for arrays as well as other containers. That doesn't work with data member function, so I added an overload for arrays as well.

I can't seem to find the comment at the top of the file that you mention here. Can you be a bit more specific about which comment you mean?

Changed the name to rawMallocRegisteredH.

I moved the sync copies out of line as well.

clang-tidy doesn't like it. clang-tidy wants them to be noexcept, but it doesn't accept the noexcept annotation if they are declared inline. I think you had some ideas about how this might make sense because default inline means that it might not be created, but default out of line was more strict.

I don't think it's a big deal that it's a bit inconsistent with the other destructors because DeviceMemorySpan doesn't own its memory like the others. I switched it to using just a single virtual call.

I think that got put there by accident. It's gone now.

Yes, I just copied the span API here. I didn't see any way for it to go without bounds checking.

It was just a leftover from the development process. It is gone now.

My understanding is that the destructor of the ThePointer member will be called when this function ends. The ThePointer member is a std::unique_ptr with a custom deleter, and the custom deleter will call out to the platform to "free" the memory owned by ThePointer. So I don't think there is any memory leak here.

So how do we access those configuration knobs now? Or, is that functionality just not available to users?

What is the purpose of having separate program and kernel classes? It seems like a kernel is just a program plus a name, so could we just merge the two? If not, maybe we can briefly explain why they are separate concepts.

Perhaps it would be consistent per elsewhere to take a Span? I don't really care either way, though; if you like it like this, that's fine.

Sorry, I think I was thinking of the commit message.

Question stands, though, wrt exactly what we're saying our library does. We can launch kernels on both CUDA and OpenCL, but only through this limited interface?

Definitely no memory leak. The question is more whether C++ requires us to do an "array placement delete" to match the "array placement new", or whether it's OK to in-place destroy the elements piecewise like we do here.

Judging from the random people on the Internet here, it seems like using array placement new for this is dicey and may actually be a buffer overflow. Maybe we should just construct the elements one at a time? Then this destruction code is clearly correct.

http://stackoverflow.com/questions/8720425/array-placement-new-requires-unspecified-overhead-in-the-buffer

The functionality is no longer available to users. We can add support back in later if needed.

The important difference is that a program can contain multiple kernels because the input source for the program can define multiple kernels. I added a comment to the Program class to explain this.

I don't think we want to merge Program with Kernel because, while it's true that a Kernel is basically just a program plus a name, it's not true that a Program is just a Kernel minus a name.

We can launch kernels on both CUDA and OpenCL through this type-unsafe interface.

Additionally, for CUDA, we can pass in our Stream and DeviceMemory types to triple-chevron kernel launches as compiled by clang or nvcc.

Oh I see! Yes, that is scary. OK, I switched to calling placement new for each element individually rather than dealing with placement new for an array.

Nit, Can we comment that this is case-insensitive?

Using a unique_ptr here means that when the program shuts down, it will run the Platform's destructor.

This is usually something we avoid in large projects because these destructors are expensive and usually not actually useful.

If we wanted to avoid running the destructor here, we could just make this into an Expected<Platform*> and unwrap the unique_ptr. I am not sure if you want that, though -- there may be actual useful work you want to do on shutdown.

std::move(Callback)

Let's expand a bit on this comment, specifically that these block the host until all previously-enqueued work on the stream plus the copy completes.

It might be worth calling these "syncCopyFoo" so there's no confusion about what they do.

Oh, I see! The comments clarify it for me, thanks.

No need to run the cleanup. I changed it to Expected<Platform *>.

Does this have to be per-thread rather than per stream? Kind of a bummer that one thread can't operate on two streams for different devices (among other things).

At least maybe setActiveDevice should be called setActiveDeviceForThread.

It looks like this map always has the keys 0, 1, ..., n. So, could we make it a vector? That will make lookups substantially cheaper, although I don't know if we care.

How does this work, exactly? We set this thread's active device index to 0, but new threads have it as uninitialized memory? That seems not so good.

Forgot to convert to seconds.

std::move(Callback)

Could we use std::array so we don't have to do these ARRAYSIZE shenanigans?

TL;DR the CUDA driver library forces us to do it per thread or to suffer a performance penalty.

I chose it to be per thread for efficiency reasons. The old StreamExecutor tried to do it per stream, and this led to extra library calls for every operation to check that the current device was equal to the one expected by the Stream. Ideally we would just store a "context" in the stream, the context would know which device it belonged to, and the context would be used to launch all work, but unfortunately that is not how the CUDA driver library works. Instead, the CUDA driver library works a lot like this where it allows you to set the active context, but then you don't pass the context to each method call, instead, it just uses the active context.

It actually doesn't prevent a thread from using streams from different devices. It just forces that thread to do the "active device" bookkeeping on its own. Want to use stream0? First set the active device to device0. Want to go back to stream1? Remember to set the active device back to device1.

I did change the name as you suggested, though.

Good point. I want every thread to start with active device set to zero because that's the way the CUDA runtime does it. I moved this initialization to the declaration of ActiveDeviceIndex. Based on some experiments and my interpretation of the standard, that should do the right thing.

I chose it to be per thread for efficiency reasons.

OK, I am totally onboard with that -- zero-cost abstractions.

But maybe we can have assert()s that ensure that users are always switching to a stream's device before touching that stream? If we have to have an ifndef NDEBUG variable or two inside the stream class to support this, that's fine by me.

We also need to have comments explaining this weird requirement.

Nit, we can get away with single braces here, which I think is more natural.

If we're using assert(), we should #error if we're built with NDEBUG, or alternatively #undef NDEBUG as the first thing in this file (before any #includes).

I wonder if we need this extra namespace (same for cuda). We don't actually expose anything other than the make-platform function in it. Do we plan on exposing anything else?

Do you want this to be explicit?

Same for CUDA: Using TLS like this means that it doesn't ever make sense for us to have two platform objects going. Which is probably correct for other reasons, too.

So maybe instead of having createFoo functions, we should have getFoo functions that return a singleton (created lazily, using static Foo* = new Foo).

I know this is basically what the current string-to-platform function is trying to do, but I guess the suggestion is to push the singleton-ness down into the lower-level factory functions, since singletonness is necessary for correctness.

This could live inside the anon ns? That's a potential speedup, too, since it lets the compiler make assumptions about this class that it can't otherwise.

Same for CUDA, if applicable (I didn't check).

s/100/MaxNumEntries/?

It's a nit, but I'm not sure we need this first if statement?

Did we decide to make streams single-threaded to avoid this overhead?

If so, in addition to getting rid of the mutex, we should get rid of the comment about concurrently enqueueing an event, since it's not valid to touch the stream concurrently at all.

I really don't like these top-level comments that say nothing you couldn't have guessed from the filename, even though we have them all over LLVM.

Does having this really help doxygen? If so, would it make doxygen look awful if we moved the comment currently on the span class into this block, so that it actually provided some value?

Can we make this constexpr?

I am really surprised that this is what they suggest in the proposal. It's fine for now, if it becomes a problem we can turn it into an assert().

Nit, could we just use data() in the decltype? We're going to use it in the constructor below anyway. Or is this as specified and the spec is just weird?

I cannot figure out the reason that some of these functions are noexcept but not others.

gcc seems to support this, at least at head: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes

s/an/a/, and below.

Can we not =default these four?

Since we're going to take ownership of the status, we should take it by value, rather than const ref (and then std::move it into TheError).

This will make it work efficiently if we're passed an rvalue.

We can do the same thing with Value -- just take it by value and std::move it.

dblaikie taught me today that we could also take Expected by value in our operator=. This would let us have
just one overload instead of two, kind of nice.

Can we use proper synchronization operations, like a condvar? volatile doesn't necessarily do what we want on all platforms -- just sort of happens to work on x86.

Wait...we have EXPECT*? Can we use this in the tests where we're currently using assert()s?

Nit, maybe we should call this arraySize, since it only works on arrays.

Nit, not sure why we have a variable for this. If you just want to make it clear what the second arg to the Span constructor means, you could write

Span(nullptr, /* size = */ 0);

I believe we even have a clang-tidy check to check comments in this style.

Nit, should be consistent wrt we have an explicit length in the array or not. I kind of prefer not. (This appears in other files as well.)

Nit, prefer constructing std::arrays with = {1, 2, 3}, as it's more natural. Also, double-braces have this truly awful footgun, which has permanently scarred those of us who have rediscovered it ourselves: https://youtu.be/rNNnPrMHsAA?t=11m15s

I don't think that assert() calls are useful to check this because the platform libraries themselves (CUDA, OpenCL, etc.) will emit errors for these cases.

I added comments to Stream to explain its associated device index and caution about using it when the active device index doesn't match, and I added a function to query the device index for a Stream.

I blame -Wall -Wextra for this. I changed it to -Wall -Wextra -Wno-missing-braces to get rid of the warning for this case.

I changed assert to "check and exit" to prevent having to deal with this.

I mostly got rid of these namespaces, but, as part of another design change, I moved the platform creation (now getter) functions into them.

Yes, I just forgot to mark it. Thanks for catching that.

Thanks for catching this. I got rid of the mutex and got rid of the whole CLEvent struct, since it is now just a cl_event*.

I also got rid of some comments for addCallback that talked about accessing the same stream from multiple threads, but I didn't see a comment that talked about Event objects being enqueued on streams concurrently. Please let me know if I didn't get rid of a comment I should have.

I don't like them either. They don't help with Doxygen as far as I know. I just put them in because I thought they were required for LLVM style. I've removed most of them now, but I did leave a few that I thought were actually serving a purpose.

Sounds good. Like you say, it should be easy to change.

I did this to try to make sure that the container supported operator[] (the proposed spec suggested I should check for this). Do you think there is a cleaner way to do that?

I don't understand it either. In the class comment I added a link the spec document I used. That's where I got all these signatures.

Unfortunately gcc only seems to support this as a function attribute (and that seems consistent with the documentation you cited). When I tried to use it as a class attribute, I got a compilation error.

Oops. Status used to be called Error.

I learned something sad while trying to do this. Status has a std::string field, and it seems that the move assignment operator for std::string is not marked noexcept in my standard library. This means that the default move assignment operator for Status is not noexcept and I get a compile error if I try to declare it noexcept and default.

To deal with this, I chose to retain the noexcept and to re-implement the default move assignment operator. I was able to default the other 3 with my standard library, so I did that.

I think I understood you correctly here. I collapsed the const T &Value and T &&Value constructors into one constructor taking the argument by value and moving it into TheValue. Let me know if I was only supposed to replace one of these constructors that way.

I couldn't find the places I am using assert() in tests. I think you might be referring to the stuff in the examples directory. That stuff doesn't depend on gtest, so it uses assert() instead.

This is a fun one. If I pass in a literal 0 it tells me the call is ambiguous because Span<int> also has a constructor with the signature Span<int>(int *FirstElem, int *LastElem). It seems that C++ is helpfully willing to convert a literal 0 to an int * (I'm sure it has to do with NULL and C compatibility).

I still thought it was a useful test, though, because the size argument could be stored in a variable as I have done in my test.

I think I got all the places where I was doing this.

These namespaces seem redundant with the function names? What are you trying to achieve by keeping these out of the root acxxel namespace? (I suspect you have a good reason...)

"platform's active device index for the thread" is awkward. This was hard for me to rephrase, but I think maybe it's better if we move the notion that it's platform-specific to the front of the paragraph, so we don't have to repeat it. Something like

Each Platform has a notion of the currently active device on a particular thread (see Platform::getActiveDeviceForThread and Platform::setActiveDeviceForThread).  Each Stream is associated with a specific, fixed device, set to the current thread's active device when we create the Stream.  Whenever a thread enqueues commands onto a Stream, its active device must match the Stream's device.

Upon reconsideration, I am not entirely sold on how we're handling errors.

If you use the fluent API, you might reasonably write code like:

Status s = stream.do_something().copy_async().sync();
if (!s.ok()) return s;

But this is subtly wrong. It will usually do what you want (return errors that occurred in any of the three operations), but if we happen to get context-switched in between the first two operations, an error from the first one may be reported when we launch the second one. In which case the sync may return OK.

(You can write effectively the same code with the non-fluent API as well.)

This problem is exacerbated by the fact that Status is now a warn-if-unused type, so if someone writes the above code and *doesn't* look at the return value, we'll warn them to!

We could fix this by making sync() return void. Alternatively, we could make sync() implicitly do a takeStatus() (we'd want to comment this). I kind of like the latter, because, presuming that you compile with clang and synchronize your streams, this makes it hard to forget error checking. We could add the warn_unused attribute to this function and then get warnings with gcc, too. (You could do that in a separate patch if you wanted.)

What do you think?

Storing it as a value type here will cause the destructor to run on program shutdown. :)

Now that I see this I like even more that we pushed the encapsulation down to these functions, because it gives you the flexibility to run code on program shutdown, or not, depending on what your platform needs.

Oh, I didn't realize that this wasn't a test, duh. Sorry about that.

As you have it is fine, or asserts are also fine, since it's just an example.

You got the comments I was after; we're good.

If the spec says to check for operator[], I'm down with this as-is.

As far as I can tell from cppreference, it's just not noexcept period.

Any reason you don't want these in the header file?

Aha, I see. That's cool, I didn't realize that clang let you warn_unused_result on a class.

Maybe this macro should have a slightly different name, to address my point of confusion. Then maybe we can have a comment to the effect of your response above?

OK, I think that's safe because std::string's move constructor doesn't throw, as I read http://en.cppreference.com/w/cpp/string/basic_string/basic_string.

Yep. It's slightly less efficient because we move value twice instead of just once, but I seriously doubt it's going to matter.

Unnecessary now that we're taking That by value.

Because we're now reading and writing it from inside a mutex, we don't need GoFlag to be an atomic. (I mean, I suppose it doesn't hurt...)

MarkerFlag is read outside a mutex, so it still needs to be an atomic, or otherwise we could just guard everything with the one mutex -- that seems like the simplest approach to me.

Hahaha, okay then. :)

Maybe not a good reason, but the reason I did it was because I also defined functions with the same name below. The functions below are wrappers for these functions, and the wrappers are responsible for returning an error if the library was compiled without support for the given platform.

To try to remove the redundancy, I changed both of these function names to just getPlatform and left the namespaces in place to distinguish which platform they are for. I'm happy to rename these functions to anything else, but I just want to preserve the public names getCUDAPlatform and getOpenCLPlatform for the wrappers.

Thanks! As you can tell, I was having a tough time explaining things there.

sync() is already doing an implicit takeStatus(), as you suggested. I didn't realize it wasn't mentioned in the comments. I've fixed the comments now.

I also added warn_if_unused the Stream functions returning a Status so that gcc can produce nice warnings too.

Now storing it as a raw pointer instead. Same change for OpenCL.

Changed the comment simply to say that the move assignment operator for std::string is noexcept.

No good reason not to have these in the header. I moved them there.

I changed the name to ACXXEL_WARN_UNUSED_RESULT_TYPE and added a comment explaining why it only works with clang.

I added the templated copy constructor to make this work (and the corresponding templated move constructor while I was at it), so now we are just returning MaybePlatform.

One thing I wasn't sure about when adding the templated constructors was whether I should remove the corresponding non-templated constructors because they now seem redundant. I didn't remove them, though, because std::unique_ptr seems to keep both the templated and non-templated version.

Yes, I think it probably makes sense to keep the non-templated constructors, although I am not 100% sure if they're necessary, like you say.