[Parallel_libs-commits] [PATCH] D24195: [SE] GlobalDeviceMemory owns its handle

[Justin Lebar via Parallel_libs-commits]

jlebar added inline comments.

================
Comment at: streamexecutor/include/streamexecutor/DeviceMemory.h:126
@@ +125,3 @@
+/// create new GlobalDeviceMemoryBase objects, and comparing them to each other
+/// for equality.
+class GlobalDeviceMemoryBase {
----------------
Is this last paragraph still fully correct?

================
Comment at: streamexecutor/include/streamexecutor/DeviceMemory.h:136
@@ +135,3 @@
+  explicit GlobalDeviceMemoryBase(Device *D, const void *Handle = nullptr,
+                                  size_t ByteCount = 0)
+      : TheDevice(D), Handle(Handle), ByteCount(ByteCount) {}
----------------
Do the handle and byte count still need to be optional?  I guess you can change it later.

================
Comment at: streamexecutor/include/streamexecutor/DeviceMemory.h:167
@@ +166,3 @@
+  GlobalDeviceMemoryBase(const GlobalDeviceMemoryBase &) = delete;
+  GlobalDeviceMemoryBase &operator=(const GlobalDeviceMemoryBase &) = delete;
+};
----------------
These don't need to be private if you don't want.

================
Comment at: streamexecutor/include/streamexecutor/DeviceMemory.h:235
@@ -234,3 +219,1 @@
 /// launch the kernel.
-class SharedDeviceMemoryBase {
-public:
----------------
Why are we getting rid of SharedDeviceMemoryBase?

I don't have a strong opinion one way or another, but it's not clear to me why this is an improvement.

The main advantage of SharedDeviceMemoryBase, I guess, is that you can accept one as a const ref parameter to a function without a template.


https://reviews.llvm.org/D24195