[Parallel_libs-commits] [PATCH] D23577: [StreamExecutor] Executor add synchronous methods

[Jason Henline via Parallel_libs-commits]

jhen added inline comments.

================
Comment at: streamexecutor/include/streamexecutor/DeviceMemory.h:148
@@ +147,3 @@
+        BaseMemory, ElementOffset + DropCount,
+        std::min(ElementCount - ElementOffset - DropCount, TakeCount));
+  }
----------------
jlebar wrote:
> Why do we do std::min here instead of asserting?
I missed that one (and it was buggy as it was anyway). Thanks for catching that!

================
Comment at: streamexecutor/include/streamexecutor/Executor.h:100
@@ +99,3 @@
+        Src.getBaseMemory(), Src.getOffset() * sizeof(T), Dst.data(), 0,
+        Src.getCount() * sizeof(T));
+  }
----------------
jlebar wrote:
> Ohhh.  *This* is why they're called getElementOffset and getElementCount.
> 
> I guess it's not so bad if we don't have any data structure that exposes the byte offset / byte count.  But even if not I find this a kind of compelling reason for those names.
> 
> What do you think?
Yes, I definitely prefer the `getElementOffset` and `getElementCount` names. Sorry about the confusion. I've changed all the names back now to include "Element".

================
Comment at: streamexecutor/include/streamexecutor/Executor.h:150
@@ -46,2 +149,2 @@
 
 #endif // STREAMEXECUTOR_EXECUTOR_H
----------------
jlebar wrote:
> > Should I add back the overloads in order to get around this problem?
> 
> Argh.  That's the common case, so I guess, yes.  I guess you only need it for the two-arg overloads and not the three-arg overload.  :-/
I put the overloads back. I couldn't even get the one that took `T*` as its second parameter to work without the overload, so I had to put them all back. I even tried to define a conversion operator for `GlobalDeviceMemory<T>` to `GlobalDeviceMemorySlice<T>`, but that didn't work either.


https://reviews.llvm.org/D23577