Page MenuHomePhabricator

Atomics: support min/max orthogonally
AcceptedPublic

Authored by t.p.northover on Dec 11 2018, 9:20 AM.

Details

Reviewers
delena
yaxunl
jfb
Summary

We seem to have been gradually growing support for atomic min/max operations (exposing longstanding IR atomicrmw instructions). But until now there have been gaps in the expected intrinsics. This adds support for the C11-style intrinsics (i.e. taking _Atomic, rather than individually blessed by C11 standard), and the variants that return the new value instead of the original one.

That way, people won't be misled by trying one form and it not working, and the front-end is more friendly to people using _Atomic types, as we recommend.

Diff Detail

Repository
rC Clang

Event Timeline

t.p.northover created this revision.Dec 11 2018, 9:20 AM
jfb added a comment.Dec 11 2018, 10:50 AM

What does it do with floating-point inputs?

What does it do with floating-point inputs?

Same as all the other atomics: run screaming for the hills (or error out, in more reasonable terms). The only way to implement floating atomics in LLVM IR would be with a compare-exchange loop and Clang doesn't expose anything to make that more convenient.

Min/max have at least some pedigree in integer terms. OpenCL has specified them, CPUs have implemented them, and we've had requests for builtins from users.

Some targets (e.g. AArch64) support 8-bit, 16-bit and 64-bit atomics. max/min in Clang 7.0 only supported 32-bit max/min, even though the other atomics supported multiple widths. Is the intention here to support all (four) offered widths, signed and unsigned, and if so, could this be reflected in the tests, which are still only testing 32-bit?

Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2019, 3:05 AM

Sorry, I managed to forget about this one somehow. I hadn't changed the 32-bit requirement, but I agree it shouldn't be there so this diff removes it and adds tests for the newly legal cases.

jfb accepted this revision.Thu, Aug 15, 9:16 AM
This revision is now accepted and ready to land.Thu, Aug 15, 9:16 AM