[Parallel_libs-commits] [PATCH] D23211: [StreamExecutor] Add DeviceMemory and kernel arg packing

[Justin Lebar via Parallel_libs-commits]

jlebar accepted this revision.
This revision is now accepted and ready to land.

================
Comment at: streamexecutor/include/streamexecutor/DeviceMemory.h:162
@@ +161,3 @@
+/// multiple SharedDeviceMemory arguments, and simply adding together all the
+/// shared memory sizes to get the final shared memory size that is used to
+/// launch the kernel.
----------------
Thanks for your answers here; sounds good to me.

================
Comment at: streamexecutor/include/streamexecutor/PackedKernelArgumentArray.h:175
@@ +174,3 @@
+  // Pack a SharedDeviceMemoryBase pointer argument.
+  void PackOneArgument(size_t Index, SharedDeviceMemoryBase *const &Argument) {
+    ++SharedCount;
----------------
Are these `FooType *const& Foo` as opposed to `FooType *Foo` for a reason?  It seems like maybe the original author was trying to "optimize" something.

================
Comment at: streamexecutor/lib/unittests/PackedKernelArgumentArrayTest.cpp:68
@@ +67,3 @@
+TEST_F(DeviceMemoryPackingTest, SingleValue) {
+  auto Array = se::make_kernel_argument_pack(Value);
+  ExpectEqual(&Value, sizeof(Value), Type::VALUE, Array, 0);
----------------
You could maybe use Twine or something, but I'm fine with it as-is.


https://reviews.llvm.org/D23211