This is an archive of the discontinued LLVM Phabricator instance.

[libc] Treat the locks array as a bitfield
ClosedPublic

Authored by jhuber6 on Jul 17 2023, 1:08 PM.

Details

Summary

Currently we keep an internal buffer of device memory that is used to
indicate ownership of a port. Since we only use this as a single bit we
can simply turn this into a bitfield. I did this manually rather than
having a separate type as we need very special handling of the masks
used to interact with the locks.

Diff Detail

Event Timeline

jhuber6 created this revision.Jul 17 2023, 1:08 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 17 2023, 1:08 PM
jhuber6 requested review of this revision.Jul 17 2023, 1:08 PM
JonChesterfield added a comment.EditedJul 18 2023, 3:39 AM

Sad to see this isn't a type. Part of the point of encapsulating the indexing and mutation ops is the inbox outbox arrays could also be bitmaps.

Even without that, there's no reason to open code the division and modulo ops. Please put those behind named functions. Set nth bit or similar.

JonChesterfield added a comment.EditedJul 18 2023, 3:45 AM
This comment has been deleted.
libc/src/__support/RPC/rpc.h
88

This isn't the size of the bitmap. I don't know what a bitfield is. Bits per word?

I'm not convinced we should pretend this stuff works for atomic types other than plain integers.

How about a static assert that atomic uint32 is the same size as a uint32 and write 32 here?

jhuber6 updated this revision to Diff 541513.Jul 18 2023, 7:07 AM

Addressing comments and moving logic to member functions.

Some nits. Still thinking about the tradeoffs between a type and functions but leaning in this direction

libc/src/__support/RPC/rpc.h
59–60

Maybe rename this while we're in the area, it's a maximum port count, not a default. Could static assert that it's a multiple of 32

161

available

248

the bitshifted boolean is probably ok - bool << unsigned probably promotes to an unsigned op - but could you either cast the cond or assign it to a uint32 before the expression to remove the ambiguity?

258

UINT32_MAX or ~0u please

jhuber6 updated this revision to Diff 541547.Jul 18 2023, 8:22 AM

Addressing comments

JonChesterfield accepted this revision.Jul 18 2023, 9:25 AM

LG, thanks

This revision is now accepted and ready to land.Jul 18 2023, 9:25 AM
This revision was automatically updated to reflect the committed changes.
jplehr added a subscriber: jplehr.Jul 19 2023, 1:45 AM

This may have broken the staging buildbot that tests libc on GPU https://lab.llvm.org/staging/#/builders/247/builds/3201

jhuber6 reopened this revision.Jul 21 2023, 8:36 AM
This revision is now accepted and ready to land.Jul 21 2023, 8:36 AM
jhuber6 updated this revision to Diff 542943.Jul 21 2023, 8:36 AM

Hopefully fixing this for the buildbot

Working theory is the bot problem was miscompiling memcpy?

Working theory is the bot problem was miscompiling memcpy?

Unsure, did some basic checks and it seems to be working now on HPE. Going to do a git pull on both RONL and HPE systems to be doubly sure.

This revision was landed with ongoing or failed builds.Jul 21 2023, 8:49 AM
This revision was automatically updated to reflect the committed changes.