This is an archive of the discontinued LLVM Phabricator instance.

[libc] Remove flexible array and replace with a template
ClosedPublic

Authored by jhuber6 on Jun 19 2023, 1:05 PM.

Details

Summary

Currently the implementation of the RPC interface requires a flexible
struct. This caused problems when compilling the RPC server with GCC as
would be required if trying to export the RPC server interface. This
required that we either move to the x[1] workaround or make it a
template parameter. While just using x[1] would be much less noisy,
this is technically undefined behavior. For this reason I elected to use
templates.

The downside to using templates is that the server code must now be able
to handle multiple different types at runtime. I was unable to find a
good solution that didn't rely on type erasure so I simply branch off of
the given value.

Diff Detail

Event Timeline

jhuber6 created this revision.Jun 19 2023, 1:05 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 19 2023, 1:05 PM
jhuber6 requested review of this revision.Jun 19 2023, 1:05 PM
jhuber6 updated this revision to Diff 532745.Jun 19 2023, 1:20 PM

Fix callback

Let me know if there's a less cumersome way to abstract around this. I gave up
because the Server type is unwiedly since it cannot really be moved or copied.

The allocation size looks like a bug. Agreed that the syntax is noisy, but on the GPU side at least it looks like a simplification to me.

libc/utils/gpu/server/Server.cpp
230

The <32> here looks suspect, should allocation size be taking a runtime value for lane size?

297

This would be much clearer if recv_and_send took lane size as a template parameter. Would delete all the copy/paste currently in the method.

jhuber6 added inline comments.Jun 19 2023, 5:51 PM
libc/utils/gpu/server/Server.cpp
230

Good catch, I'll fix it.

jhuber6 added inline comments.Jun 19 2023, 5:55 PM
libc/utils/gpu/server/Server.cpp
297

This is supposed to be used externally so we can't put it in a template. The difficulty is mainly casting the opaque wrapper to the proper type. I don't like how ugly it all is, maybe I'll think of a better solution.

jhuber6 updated this revision to Diff 533017.Jun 20 2023, 12:28 PM

Somewhat simplify interface, use a variant to unique pointers.

JonChesterfield accepted this revision.Jun 20 2023, 1:13 PM

I think this is alright to be honest. It does something subtle fairly cleanly.

libc/utils/gpu/server/Server.cpp
227

switch?

241

we might move allocation_size to a free function at some point, it doesn't really need to be templated on lane size

This revision is now accepted and ready to land.Jun 20 2023, 1:13 PM
This revision was automatically updated to reflect the committed changes.