[Parallel_libs-commits] [PATCH] D23038: [StreamExecutor] Add KernelLoaderSpec

[Jason Henline via Parallel_libs-commits]

jhen added inline comments.

================
Comment at: streamexecutor/include/streamexecutor/KernelSpec.h:144
@@ +143,3 @@
+/// A KernelLoaderSpec for CUDA fatbin code that resides in memory as a
+/// null-terminated string.
+class CUDAFatbinInMemorySpec : public KernelLoaderSpec {
----------------
jlebar wrote:
> It's not a null-terminated string, I think.  I thought it was at first, but I think I was being mislead by "char" -- it's more likely that the first 4 bytes are the length (or something).
Oops. I just wrote that comment in the wrong place. You're right that the char types are making things confusing.

================
Comment at: streamexecutor/include/streamexecutor/KernelSpec.h:178
@@ +177,3 @@
+private:
+  const char *Text;
+
----------------
jlebar wrote:
> Hm...what if we used char* for text strings and uint8_t* for byte data?
Yes, that is much better. How do you feel about void* rather than uint8_t? I think that would be good because the data is a formless blob.

================
Comment at: streamexecutor/lib/unittests/KernelSpecTest.cpp:74
@@ +73,3 @@
+  ASSERT_DEATH(MultiSpec.getOpenCLTextInMemory(),
+               "getting spec that is not present");
+#endif
----------------
jlebar wrote:
> EXPECT_DEBUG_DEATH and you don't need the ifdefs.  Same below.
Ooooooo, that's nice. Thanks!


https://reviews.llvm.org/D23038