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

[Jason Henline via Parallel_libs-commits]

jhen 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 {
----------------
jlebar wrote:
> Is this last paragraph still fully correct?
No, it was not. I just removed it completely because I think it's not needed anyway.

================
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) {}
----------------
jlebar wrote:
> Do the handle and byte count still need to be optional?  I guess you can change it later.
I changed it now. They don't need to be optional.

================
Comment at: streamexecutor/include/streamexecutor/DeviceMemory.h:167
@@ +166,3 @@
+  GlobalDeviceMemoryBase(const GlobalDeviceMemoryBase &) = delete;
+  GlobalDeviceMemoryBase &operator=(const GlobalDeviceMemoryBase &) = delete;
+};
----------------
jlebar wrote:
> These don't need to be private if you don't want.
I'm never sure where is the best place to put these because the visibility doesn't seem to matter. I moved them into the public section to prevent creating a new section for them. Does that seem cleaner?

================
Comment at: streamexecutor/include/streamexecutor/DeviceMemory.h:235
@@ -234,3 +219,1 @@
 /// launch the kernel.
-class SharedDeviceMemoryBase {
-public:
----------------
jlebar wrote:
> 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.
I think its an improvement because we ain't gonna need it. If it becomes useful later on, we can add it back in, but it doesn't seem like it will.


https://reviews.llvm.org/D24195