This is an archive of the discontinued LLVM Phabricator instance.

[StreamExecutor] Simplify Kernel classes
ClosedPublic

Authored by jhen on Aug 30 2016, 10:50 AM.

Details

Summary

Make the Kernel class follow the pattern of the other classes. It now
has a type-safe user wrapper and a typeless, platform-specific handle.

Diff Detail

Event Timeline

jhen updated this revision to Diff 69724.Aug 30 2016, 10:50 AM
jhen retitled this revision from to [StreamExecutor] Simplify Kernel classes.
jhen updated this object.
jhen added a reviewer: jlebar.
jhen added subscribers: parallel_libs-commits, jprice.
jprice added inline comments.Aug 30 2016, 1:32 PM
streamexecutor/include/streamexecutor/Kernel.h
48–51

Maybe I'm missing something, but should these parameter types actually be GlobalDeviceMemory<float>? Otherwise I'm not sure how such a kernel object could be passed to a thenLaunch() function with the correct arguments.

63–64

Missing std::unique_ptr<> here?

64–65

cnn -> ccn

jhen updated this revision to Diff 69752.Aug 30 2016, 2:04 PM
jhen marked 3 inline comments as done.
  • Fix comment bugs found by jprice
streamexecutor/include/streamexecutor/Kernel.h
48–51

You're right. Plus the A parameter is usually passed as a float not a float *. Now the signature should be fixed.

jlebar accepted this revision.Aug 30 2016, 4:00 PM
jlebar edited edge metadata.

Well, that does look simpler.

Thanks a lot for all these reviews, jprice.

streamexecutor/include/streamexecutor/Device.h
33–50

Can we be explicit that if this doesn't return an error, it is guaranteed not to return a null value?

This revision is now accepted and ready to land.Aug 30 2016, 4:00 PM
jhen updated this revision to Diff 69779.Aug 30 2016, 4:21 PM
jhen edited edge metadata.
  • Add never-null comment to Device::createKernel
jhen marked an inline comment as done.Aug 30 2016, 4:23 PM

Thanks a lot for all these reviews, jprice.

I second those sentiments. You've been a great help, jprice.

streamexecutor/include/streamexecutor/Device.h
33–50

Good idea.

This revision was automatically updated to reflect the committed changes.
jhen marked an inline comment as done.
streamexecutor/lib/unittests/CMakeLists.txt