Patchwork Adjust omp-low test for alignment

login
register
mail settings
Submitter Richard Henderson
Date Nov. 27, 2011, 12:05 a.m.
Message ID <4ED17EB4.4070807@twiddle.net>
Download mbox | patch
Permalink /patch/127823/
State New
Headers show

Comments

Richard Henderson - Nov. 27, 2011, 12:05 a.m.
The m68k-linux failure for the various omp atomic tests
is due to the fact that BIGGEST_ALIGNMENT is 16 bits on
that platform.  I think it's pretty reasonable to assume
that if something is aligned to BIGGEST_ALIGNEMENT, then
it can be considered "aligned".

Tested on x86_64-linux and m68k-linux cross.


r~
* omp-low.c (expand_omp_atomic): Assume anything aligned to
        BIGGEST_ALIGNMENT is aligned.
Hans-Peter Nilsson - Nov. 29, 2011, 4:49 a.m.
On Sat, 26 Nov 2011, Richard Henderson wrote:
> The m68k-linux failure for the various omp atomic tests
> is due to the fact that BIGGEST_ALIGNMENT is 16 bits on
> that platform.  I think it's pretty reasonable to assume
> that if something is aligned to BIGGEST_ALIGNEMENT, then
> it can be considered "aligned".

BIGGEST_ALIGNMENT means aligned enough for normal access, but
not necessarily for atomic access.

This particular fix wouldn't do it for CRIS, for example, were
BIGGEST_ALIGNMENT is 8 (bits), but an atomic access (inside the
ll/sc sequence) such as for a futex requires not straddling a
page boundary, i.e. "natural" alignment.

Not that OMP support is imminent or critical for cris-linux or
anything, but can we have a new macro?

brgds, H-P
Richard Henderson - Nov. 29, 2011, 6:51 p.m.
On 11/28/2011 08:49 PM, Hans-Peter Nilsson wrote:
> On Sat, 26 Nov 2011, Richard Henderson wrote:
>> The m68k-linux failure for the various omp atomic tests
>> is due to the fact that BIGGEST_ALIGNMENT is 16 bits on
>> that platform.  I think it's pretty reasonable to assume
>> that if something is aligned to BIGGEST_ALIGNEMENT, then
>> it can be considered "aligned".
> 
> BIGGEST_ALIGNMENT means aligned enough for normal access, but
> not necessarily for atomic access.

If that's true, then you'll have problems applying any of these
functions without additional source-code level alignment, everywhere.

> Not that OMP support is imminent or critical for cris-linux or
> anything, but can we have a new macro?

I'm not sure what you're suggesting that the macro actually do.


r~
Hans-Peter Nilsson - Nov. 29, 2011, 9:12 p.m.
On Tue, 29 Nov 2011, Richard Henderson wrote:
> On 11/28/2011 08:49 PM, Hans-Peter Nilsson wrote:
> > On Sat, 26 Nov 2011, Richard Henderson wrote:
> >> The m68k-linux failure for the various omp atomic tests
> >> is due to the fact that BIGGEST_ALIGNMENT is 16 bits on
> >> that platform.  I think it's pretty reasonable to assume
> >> that if something is aligned to BIGGEST_ALIGNEMENT, then
> >> it can be considered "aligned".
> >
> > BIGGEST_ALIGNMENT means aligned enough for normal access, but
> > not necessarily for atomic access.
>
> If that's true,

It's what that macro meant up until gcc started to be
atomicity-aware at this level, as implied by "when violated, may
cause a fault".  Changing it to higher makes gcc do all stupid
things when accessing structure members with lower alignment so
I can't do that, it violates the byte-aligment ABI.

> then you'll have problems applying any of these
> functions without additional source-code level alignment, everywhere.

There has to be a type that matches the (let's call it)
ATOMIC_ALIGNMENT yes, is that what you mean by "any of these
functions"?

> > Not that OMP support is imminent or critical for cris-linux or
> > anything, but can we have a new macro?
>
> I'm not sure what you're suggesting that the macro actually do.

Tell proper aligmnent for atomic access, defaulting to (say)
natural aligmnent.

brgds, H-P
Hans-Peter Nilsson - Dec. 2, 2011, 1:12 p.m.
On Tue, 29 Nov 2011, Hans-Peter Nilsson wrote:

> On Tue, 29 Nov 2011, Richard Henderson wrote:
> > On 11/28/2011 08:49 PM, Hans-Peter Nilsson wrote:
> > > On Sat, 26 Nov 2011, Richard Henderson wrote:
> > >> The m68k-linux failure for the various omp atomic tests
> > >> is due to the fact that BIGGEST_ALIGNMENT is 16 bits on
> > >> that platform.  I think it's pretty reasonable to assume
> > >> that if something is aligned to BIGGEST_ALIGNEMENT, then
> > >> it can be considered "aligned".
> > >
> > > BIGGEST_ALIGNMENT means aligned enough for normal access, but
> > > not necessarily for atomic access.
> >
> > If that's true,
>
> It's what that macro meant up until gcc started to be
> atomicity-aware at this level, as implied by "when violated, may
> cause a fault".  Changing it to higher makes gcc do all stupid
> things when accessing structure members with lower alignment so
> I can't do that, it violates the byte-aligment ABI.
>
> > then you'll have problems applying any of these
> > functions without additional source-code level alignment, everywhere.
>
> There has to be a type that matches the (let's call it)
> ATOMIC_ALIGNMENT yes, is that what you mean by "any of these
> functions"?

Oh, on second reading I see you probably mean I have to make
sure the atomic types are aligned in the library, by e.g.
attaching __attribute__ ((__aligned__)).  Sure: the reply to
this change in the gut of gcc is however more important to make
sure it's not cast in stone and copied to other places that I'll
only find the hard way.  BTW, on the topic, I cringe whenever I
see futexes expressed as plain "int", they absolutely have to
have at least natural alignment which is not always true e.g. in
structs.  People, please keep the atomic types
target-overridable in libraries.

> > > Not that OMP support is imminent or critical for cris-linux or
> > > anything, but can we have a new macro?
> >
> > I'm not sure what you're suggesting that the macro actually do.
>
> Tell proper aligmnent for atomic access, defaulting to (say)
> natural aligmnent.
>
> brgds, H-P
>
Mikael Pettersson - Dec. 2, 2011, 3:33 p.m.
Hans-Peter Nilsson writes:
 >   BTW, on the topic, I cringe whenever I
 > see futexes expressed as plain "int", they absolutely have to
 > have at least natural alignment which is not always true e.g. in
 > structs.  People, please keep the atomic types
 > target-overridable in libraries.

+1 for m68k-linux, where plain "int" only has 16-bit alignment
(by SW convention, Linux-capable HW tolerates 8-bit alignment),
but futexes must be 32-bit aligned (or at least not cross page
boundaries).
Hans-Peter Nilsson - Dec. 2, 2011, 8:14 p.m.
On Fri, 2 Dec 2011, Mikael Pettersson wrote:
> but futexes must be 32-bit aligned (or at least not cross page
> boundaries).

Don't mix up futexes with hardware-mandated atomic alignment
(except that preferably the letter should not be more strict).

Linux futexes must be 32-bit aligned *for all architectures*.

Linux uses the two low bits for its own purposes, when it stores
futex addresses internally (or something to that effect; it was
a while ago I looked at this, around Feb 2009 alt. 2.6.27-ish).

brgds, H-P
Andreas Krebbel - Feb. 1, 2012, 8:22 a.m.
On Sat, Nov 26, 2011 at 04:05:08PM -0800, Richard Henderson wrote:
> The m68k-linux failure for the various omp atomic tests
> is due to the fact that BIGGEST_ALIGNMENT is 16 bits on
> that platform.  I think it's pretty reasonable to assume
> that if something is aligned to BIGGEST_ALIGNEMENT, then
> it can be considered "aligned".
> 
> Tested on x86_64-linux and m68k-linux cross.
> 
> 
> r~

>         * omp-low.c (expand_omp_atomic): Assume anything aligned to
>         BIGGEST_ALIGNMENT is aligned.

That broke the atomic-2.c libgomp testcase on s390x. We have
BIGGEST_ALIGNMENT of 64. A 128 bit long double does not need to be
aligned better than 64 bit in memory. However, the 128bit compare and
swap instruction we have requires the operands to be naturally
aligned. In the testcase a compare and swap double instruction is used
on a long double value which is only 8 byte aligned in memory.

This seem to have caused problems on CRIS as well.  The proposed
solution was to force the alignment of the types using that aligned
attribute. While this is a good idea anyway in order to get the proper
hardware instructions, I think omp-low should be able to pick a
fallback solution if necessary.  Otherwise, we will silently insert
bugs which are difficult to find. The atomic-2 testcase for example
succeeds very often since the long double happens to be naturally
aligned in a lot of cases. So to my understanding the change above is
incorrect.

Bye,

-Andreas-

Patch

diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index a4bfb84..4e1c2ba 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -5501,7 +5501,9 @@  expand_omp_atomic (struct omp_region *region)
       unsigned int align = TYPE_ALIGN_UNIT (type);
 
       /* __sync builtins require strict data alignment.  */
-      if (exact_log2 (align) >= index)
+      /* ??? Assume BIGGEST_ALIGNMENT *is* aligned.  */
+      if (exact_log2 (align) >= index
+	  || align * BITS_PER_UNIT >= BIGGEST_ALIGNMENT)
 	{
 	  /* Atomic load.  */
 	  if (loaded_val == stored_val