This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Use type alignment in get_builtin_sync_mem

On September 9, 2019 4:17:20 PM GMT+02:00, Ulrich Weigand <> wrote:
>Richard Biener wrote:
>> On Fri, Sep 6, 2019 at 3:00 PM Ulrich Weigand <>
>> > But as far as I can see, for *atomic* operations at least, we do
>> > that assumption.  The x86 back-end for example just assumes that
>> > "int" or "long" object that is the target of an atomic operation is
>> > naturally aligned, or else the generated code would just crash.  
>> > if you used your example with a packed struct and passed that
>> > to an atomic, the back-end would still assume the alignment and the
>> > code would crash.  But I'd still consider this a perfectly
>> > and expected behavior in this case ...
>> Would it crash?  I think it would be not atomic if it crossed a
>> boundary.
>Sorry, I misremembered the x86 operations, it does indeed work for
>unaligned 4- or 8-byte accesses.  However, for 16-byte accesses,
>CMPXCHG16B does require aligned memory, the manual says:
>  Note that CMPXCHG16B requires that the destination (memory)
>  operand be 16-byte aligned.
>64-Bit Mode Exceptions
>#GP(0) If the memory address is in a non-canonical form.
>       If memory operand for CMPXCHG16B is not aligned on a
>       16-byte boundary.
>So this is basically the same situation as on s390, except that on x86
>the default TImode alignment is already 16 bytes.
>> > Of course if some part of the middle end get the alignment wrong,
>> > have a problem.  But I'm not sure how this could happen here.  I
>> > that it might be the case that a user-specified *under*-alignment
>> > get lost somewhere (e.g. you have a packed int and somewhere in the
>> > optimizers this gets accidentally cast to a normal int with the
>> > alignment).  But in *this* case, what would have to happen is that
>> > middle-end accidentally casts something to a pointer with *higher*
>> > than the default alignment for the type, even though no such
>> > cast is present in the source.  Can this really happen?
>> If the cast to the lower-aligned type is lost and there is an earlier
>> to the aligned type.
>My point is that this "cast to the aligned type" must have come from
>user in this case (since the aligned type is *more* aligned that any
>standard version of the typ), and if the user casts the value to the
>type, it is undefined behavior anyway if the value was in fact not
>> > This would actually
>> > be wrong on s390.  The problem is that all atomic operations on any
>> > one single object need to be consistent: they either all use the
>> > 16-byte atomic instruction, or else they all protect the access
>> > a lock.  If you have parts of the code use the lock and other parts
>> > use the instruction, they're not actually protected against each
>> But then the user has to be consistent in accessing the object
>> atomically.  If he accesses it once as (aligned_int128_t *)
>> and once as (int128_t *) it's his fault, no?
>I'm not sure why this should be a requirement.  E.g. if we have a
>set of subroutines that operates (correctly) on any int128_t *,
>aligned or not, and have one user of those routines that actually
>locally has an aligned_int128_t, then that user could no longer
>safely pass that a pointer to its aligned variable to that
>subsystem if it also does atomic operations locally ...
>> If we'd document that the user invokes undefined behavior
>> when performing __builtin_atomic () on objects that are not
>> sufficiently aligned according to target specific needs then
>> we are of course fine and should simply assume the memory
>> is aligned accordingly (similar to your patch but probably with
>> some target hook specifying the atomic alignment requirement
>> when it differs from mode alignment).  But I don't read the
>> documentation of our atomic builtins that way.
>> Does _Atomic __int128_t work properly on s390?
>Yes, it currently does work properly in all cases (just not in
>all cases as efficiently as it could be).
>The rule to perform atomic operations on __int128_t on s390 is:
> - If the object is *actually* 16-byte aligned at runtime, then
>   every atomic access must be performed using one of the atomic
>   instructions (CDSG, LPQ, STPQ).
> - If the object is actually *not* 16-byte aligned, then every
>   atomic access must be performed under protection of an
>   out-of-line lock.
>This rule is correctly implemented by:
> - The __builtin_*_16 family of libatomic library routines;
>   these all perform a run-time alignment check and use either
>   the instruction or the lock, as appropriate; and
> - Compiler-generated inline code;
>   this will always use the instruction, but the compiler will
>   generate it only if it can prove at compile-time that the
>   object *must* be aligned at run-time.
>[ However, this rule is *not* implemented by the _n family of
>libatomic library routines.  That is not a problem at the moment
>since those will *never* get called on any object of size 16;
>but they would be if we implemented your proposal; that's why
>I pointed out that this would then *introduce* unsafe behavior. ]
>The original point of why I started this discussion is nothing to
>do with safety -- code is currently safe and would remain safe
>after this change -- but simply performance: we would get compiler
>generated inline code more frequently than we do today.
>(This is not *just* performance but also convenience, since we
>can avoid having to explicitly link in libatomic in many cases
>as well.  This would help with portability of some Makefiles
>etc. to s390, where they don't link in libatomic since it is
>not needed on x86 ...)
>> > This is why the _16 version of the library routine does the runtime
>> > alignment check, so that all accesses to actually 16-byte aligned
>> > objects use the instruction, both in the library and in compiler-
>> > generated code.   The _n version doesn't do that.
>> >
>> > So I guess we'd have to convert the _n version back to the _16
>> > library call if the size is constant 16, or something?
>> If it is aligned 16?  But not sure - see above.
>No, it would have to be done no matter whether it is (i.e. can be
>proven at compiler-time) aligned or not, the point is we must call
>the _16 version of the library routine so that it can do the run-time
>alignment check and perform the appropriate action.
>> > > So yes, I'd say try tackling the issue in the frontend which is
>> > > last to know the alignment in the most optimistic way (according
>> > > to the C standard).  At RTL expansion time we have to be (very)
>> > > conservative.
>> > >
>> > > Btw, the alternative is to add an extra alignment parameter
>> > > to all of the atomic builtins where the FE could stick the
>> > > for us to use during RTL expansion.  But given we have the
>> > > variants it sounds easier to use those...
>> > >
>> > > Note we only document __atomic_load and __atomic_load_n so
>> > > the _{1,2,4,8,16} variants seem to be implementation detail.
>> >
>> > Adding the alignment parameter would work, I think.
>> But then the user still needs to access the int128_t with
>> consistent alignment, no?  Otherwise it will again not protect
>> against each other?
>See above, it will still work *correctly* in either case.  The
>alignment info simply allows the compiler to safely choose the
>fast (inlined) variant. 
>> Why not always use the slow & correct variant for __int128?
>As above, both slow & fast variants are correct; it's just that
>the fast variant is, well, faster and more convenient.

So the compiler already does that correctly then via get_pointer_alignment. 
Why not inline the runtime alignment check as well plus the fast case and the library call fallback? 



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]