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

[Jason Henline via Parallel_libs-commits]

jhen added inline comments.

================
Comment at: streamexecutor/include/streamexecutor/PackedKernelArgumentArray.h:175
@@ +174,3 @@
+  // Pack a SharedDeviceMemoryBase pointer argument.
+  void PackOneArgument(size_t Index, SharedDeviceMemoryBase *const &Argument) {
+    ++SharedCount;
----------------
jlebar wrote:
> 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.
I was actually the author of that mess. The original code uses SFINAE stuff that I didn't like and during development of this code I thought I needed those ugly types to get the template type inference to work right. But I tried it with the simpler types just now and it worked fine, so I don't know what was going wrong for me before.

As a result, I got rid of the ugly types and am now just using the pointer types. Thanks pointing out that I could do that!

================
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);
----------------
jlebar wrote:
> You could maybe use Twine or something, but I'm fine with it as-is.
The Twine works nicely. Thanks for the suggestion.


https://reviews.llvm.org/D23211