[PATCH] D24609: [ARM] Add missing Interlocked intrinsics
Martin Storsjö via cfe-commits
cfe-commits at lists.llvm.org
Sat Sep 17 03:16:08 PDT 2016
mstorsjo added inline comments.
================
Comment at: lib/Headers/intrin.h:504
@@ +503,3 @@
+_interlockedbittestandset_acq(long volatile *_BitBase, long _BitPos) {
+ long _PrevVal = __atomic_fetch_or(_BitBase, 1l << _BitPos, __ATOMIC_ACQUIRE);
+ return (_PrevVal >> _BitPos) & 1;
----------------
compnerd wrote:
> mstorsjo wrote:
> > compnerd wrote:
> > > Perhaps we should add static asserts that _BitPos is within limits for the signed shift.
> > 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.
> 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.
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.
https://reviews.llvm.org/D24609
More information about the cfe-commits
mailing list