This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Add missing Interlocked intrinsics
ClosedPublic

Authored by mstorsjo on Sep 15 2016, 5:46 AM.

Details

Reviewers
compnerd
Summary

On ARM, there are multiple versions of each of the intrinsics, with acquire/relaxed/release barrier semantics.

The 64 bit versions that so far were within "ifdef x86_64" also are necessary for arm - I think the reason why they were in ifdefs is that winnt.h have got inline versions of them for, within ifdef _M_IX86.

The newly added ones are provided as inline functions here instead of builtins, since they should only be available on certain archs (arm/aarch64).

This is necessary in order to compile C++ code for ARM in MSVC mode.

Diff Detail

Event Timeline

mstorsjo updated this revision to Diff 71497.Sep 15 2016, 5:46 AM
mstorsjo retitled this revision from to [ARM] Add missing Interlocked intrinsics.
mstorsjo updated this object.
mstorsjo added a reviewer: compnerd.
mstorsjo added a subscriber: cfe-commits.
compnerd added inline comments.Sep 15 2016, 8:49 PM
lib/Headers/intrin.h
504

Perhaps we should add static asserts that _BitPos is within limits for the signed shift.

517

Are the names supposed to be all lower case?

mstorsjo added inline comments.Sep 16 2016, 2:41 AM
lib/Headers/intrin.h
504

Sure, although I guess that also goes for the existing inline functions as well?

Which kind of assert would be suitable for that here? As far as I see, static_assert is C++ only, while this header also can be used from C.

If I try to add _Static_assert, which is usable in C, I get the following error when compiling:

intrin.h:499:18: error:

    static_assert expression is not an integral constant expression
_Static_assert(_BitPos < 32, "_BitPos out of range");

This even when I don't actually use the inline function anywhere, just including intrin.h.

517

Yes, these ones (for some reason) are all lower case:
https://msdn.microsoft.com/en-us/library/646k06sz.aspx

compnerd added inline comments.Sep 16 2016, 10:33 PM
lib/Headers/intrin.h
504

Yeah, we would have to use _Static_assert, but that requires C11. It is possible to emulate it, but probably not worth the effort. I am worried though that we could introduce undefined behavior with an incorrect parameter.

mstorsjo added inline comments.Sep 17 2016, 3:16 AM
lib/Headers/intrin.h
504

Sure, there's probably such a risk. But this issue already exists - an almost identical _interlockedbittestandset function already exists in the header - I'm just adding new copies of it with different atomic semantics (__ATOMIC_ACQUIRE etc).

So I'd ask if we can deal with that issue separately from this patch.

compnerd accepted this revision.Sep 19 2016, 10:01 PM
compnerd edited edge metadata.

That sounds reasonable.

This revision is now accepted and ready to land.Sep 19 2016, 10:01 PM

Thanks! Can you/someone commit it?

Im happy to do so, but, could you please rebase the patch?

mstorsjo updated this revision to Diff 72025.Sep 21 2016, 4:43 AM
mstorsjo updated this object.
mstorsjo edited edge metadata.

Rebased on top of the latest master

Can someone commit this patch now after it has been rebased?

Ping, can someone commit this one now?

compnerd closed this revision.Sep 26 2016, 3:38 PM

r282447