Re: [PATCH] Use type alignment in get_builtin_sync_mem

["Ulrich Weigand" <uweigand@de.ibm.com>]

Richard Biener wrote:
> On Fri, Sep 6, 2019 at 3:00 PM Ulrich Weigand <uweigand@de.ibm.com> wrote:
> > But as far as I can see, for *atomic* operations at least, we do make
> > that assumption.  The x86 back-end for example just assumes that any
> > "int" or "long" object that is the target of an atomic operation is
> > naturally aligned, or else the generated code would just crash.   So
> > if you used your example with a packed struct and passed that pointer
> > to an atomic, the back-end would still assume the alignment and the
> > code would crash.  But I'd still consider this a perfectly reasonable
> > and expected behavior in this case ...
> 
> Would it crash?  I think it would be not atomic if it crossed a cache-line
> 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, we
> > have a problem.  But I'm not sure how this could happen here.  I agree
> > that it might be the case that a user-specified *under*-alignment might
> > 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 default
> > alignment).  But in *this* case, what would have to happen is that the
> > middle-end accidentally casts something to a pointer with *higher*
> > than the default alignment for the type, even though no such pointer
> > 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 cast
> to the aligned type.

My point is that this "cast to the aligned type" must have come from the
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 aligned
type, it is undefined behavior anyway if the value was in fact not aligned.

> > 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 with
> > 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 other.
> 
> 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 the
> > > 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 alignment
> > > for us to use during RTL expansion.  But given we have the _{1,2,4,8,16,N}
> > > 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.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com