This is an archive of the discontinued LLVM Phabricator instance.

[SE] Remove Platform*Handle classes
ClosedPublic

Authored by jhen on Sep 2 2016, 6:27 PM.

Details

Summary

As pointed out by jprice, these classes don't serve a purpose. Instead,
we stay consistent with the way memory is managed and let the Stream and
Kernel classes directly hold opaque handles to device Stream and Kernel
instances, respectively.

Diff Detail

Event Timeline

jhen updated this revision to Diff 70261.Sep 2 2016, 6:27 PM
jhen retitled this revision from to [SE] Remove Platform*Handle classes.
jhen updated this object.
jhen added reviewers: jprice, jlebar.
jhen added a subscriber: parallel_libs-commits.
jlebar accepted this revision.Sep 4 2016, 3:14 PM
jlebar edited edge metadata.

Seems simpler to me!

streamexecutor/lib/Kernel.cpp
29

Can we assert that PDevice and PlatformKernelHandle are not null? Otherwise we'll find out that PDevice was null only when we destroy the KernelBase.

streamexecutor/lib/Stream.cpp
21–46

Same for assertions here.

This revision is now accepted and ready to land.Sep 4 2016, 3:14 PM
jprice accepted this revision.Sep 5 2016, 2:21 AM
jprice edited edge metadata.

Looks good to me.

If there are never going to be any other platform interfaces I guess it might make sense to rename PlatformInterfaces.* to just PlatformDevice.*, but maybe it's too early for that.

jhen updated this revision to Diff 70426.Sep 6 2016, 10:08 AM
jhen marked 2 inline comments as done.
jhen edited edge metadata.
  • assert no nulls in ctors
jhen added a comment.Sep 6 2016, 10:08 AM

If there are never going to be any other platform interfaces I guess it might make sense to rename PlatformInterfaces.* to just PlatformDevice.*, but maybe it's too early for that.

I agree that PlatformDevice is a a better name now. I'll get a patch up for that after this one goes in.

jhen updated this revision to Diff 70427.Sep 6 2016, 10:13 AM
  • dummy platform stream in Stream test
This revision was automatically updated to reflect the committed changes.