MIPS atomic memory operations (A.K.A PR 33479).

MIPS atomic memory operations (A.K.A PR 33479).

[David Daney]

Richard,

There seems to be a small problem with the MIPS atomic memory operations 
patch I recently committed 
(http://gcc.gnu.org/ml/gcc-patches/2007-08/msg01290.html), in that on a 
dual CPU machine it does not quite work.

You can look at http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33479#c3 for 
more information.

Here is the code in question (from mips.h):

#define MIPS_COMPARE_AND_SWAP(SUFFIX, OP)	\
   "%(%<%[sync\n"				\
   "1:\tll" SUFFIX "\t%0,%1\n"			\
   "\tbne\t%0,%2,2f\n"				\
   "\t" OP "\t%@,%3\n"				\
   "\tsc" SUFFIX "\t%@,%1\n"			\
   "\tbeq\t%@,%.,1b\n"				\
   "\tnop\n"					\
   "2:%]%>%)"



I guess my basic question is:  Should MIPS_COMPARE_AND_SWAP have a 
'sync' after the 'sc'?  I would have thought that 'sc' made the write 
visible to all CPUs, but on the SB1 it appears not to be the case.

If we do need to add another 'sync' should it go in the delay slot of 
the branch?  I would say yes because we would expect the branch to 
rarely taken.

Any feedback from linux-mips people is also solicited.

Thanks,
David Daney

Re: MIPS atomic memory operations (A.K.A PR 33479).

[Daniel Jacobowitz]

On Tue, Sep 18, 2007 at 05:12:48PM -0700, David Daney wrote:
> I guess my basic question is:  Should MIPS_COMPARE_AND_SWAP have a 'sync' after 
> the 'sc'?  I would have thought that 'sc' made the write visible to all CPUs, 
> but on the SB1 it appears not to be the case.

Yes, a barrier of some sort is definitely necessary.  I believe the
SB1 is weakly ordered, and the architecture spec permits both strong
and weak ordering; but it's been a while since I tried this.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MIPS atomic memory operations (A.K.A PR 33479).

[Thiemo Seufer]

David Daney wrote:
> Richard,
>
> There seems to be a small problem with the MIPS atomic memory operations 
> patch I recently committed 
> (http://gcc.gnu.org/ml/gcc-patches/2007-08/msg01290.html), in that on a 
> dual CPU machine it does not quite work.
>
> You can look at http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33479#c3 for 
> more information.
>
> Here is the code in question (from mips.h):
>
> #define MIPS_COMPARE_AND_SWAP(SUFFIX, OP)	\
>   "%(%<%[sync\n"				\

This sync is for SB-1 implied by ll, but other MIPS systems may
need it.

>   "1:\tll" SUFFIX "\t%0,%1\n"			\
>   "\tbne\t%0,%2,2f\n"				\
>   "\t" OP "\t%@,%3\n"				\
>   "\tsc" SUFFIX "\t%@,%1\n"			\
>   "\tbeq\t%@,%.,1b\n"				\
>   "\tnop\n"					\

The SB-1 needs a "sync" here.

>   "2:%]%>%)"
>
>
>
> I guess my basic question is:  Should MIPS_COMPARE_AND_SWAP have a 'sync' 
> after the 'sc'?  I would have thought that 'sc' made the write visible to 
> all CPUs, but on the SB1 it appears not to be the case.
>
> If we do need to add another 'sync' should it go in the delay slot of the 
> branch?  I would say yes because we would expect the branch to rarely 
> taken.

But it would make things a lot worse for the contended case, which is
the interesting one for performance.


Thiemo

Re: MIPS atomic memory operations (A.K.A PR 33479).

[Ralf Baechle]

On Tue, Sep 18, 2007 at 05:12:48PM -0700, David Daney wrote:

> There seems to be a small problem with the MIPS atomic memory operations 
> patch I recently committed 
> (http://gcc.gnu.org/ml/gcc-patches/2007-08/msg01290.html), in that on a 
> dual CPU machine it does not quite work.
> 
> You can look at http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33479#c3 for 
> more information.
> 
> Here is the code in question (from mips.h):
> 
> #define MIPS_COMPARE_AND_SWAP(SUFFIX, OP)	\
>   "%(%<%[sync\n"				\
>   "1:\tll" SUFFIX "\t%0,%1\n"			\
>   "\tbne\t%0,%2,2f\n"				\
>   "\t" OP "\t%@,%3\n"				\
>   "\tsc" SUFFIX "\t%@,%1\n"			\
>   "\tbeq\t%@,%.,1b\n"				\

Please make this loop closure branch a branch-likely.  This is necessary
as a errata workaround for some processors.

(I know silicon people hate me for keeping branch likely instruction alive
this way but it's my job to make sure Linux and software are working without
unpleasant surprises.)

>   "\tnop\n"					\
>   "2:%]%>%)"
> 
> 
> 
> I guess my basic question is:  Should MIPS_COMPARE_AND_SWAP have a 
> 'sync' after the 'sc'?  I would have thought that 'sc' made the write 
> visible to all CPUs, but on the SB1 it appears not to be the case.

The MIPS architecture specification specifies no memory model, so for
portable code you need to make a worst case assumption which is weak
ordering.

Only on R4000 and R4400 SC and SCD did imply a SYNC operation.

> If we do need to add another 'sync' should it go in the delay slot of 
> the branch?  I would say yes because we would expect the branch to 
> rarely taken.

Not when using a branch likely.

Btw, I recently wrote an article about memory consistency which is at
http://www.linux-mips.org/wiki/Memory_consistency.  It gives a bit of
an overview of things in general and on MIPS specifically.  I request
people with detailed knowledge of MIPS cores not specifically covered
in that article to contribute.

  Ralf

Re: MIPS atomic memory operations (A.K.A PR 33479).

[Maciej W. Rozycki]

On Wed, 19 Sep 2007, Ralf Baechle wrote:

> Please make this loop closure branch a branch-likely.  This is necessary
> as a errata workaround for some processors.

 Do we emulate them for MIPS I?  We do emulate "ll" and "sc" and adding 
"sync" is easy (as a no-op as support for R3000 SMP is unlikely to ever 
happen).  Adding branches-likely, hmm...  Even though we do have logic to 
do that as a part of the FP emulator.

 A workaround for a CPU erratum fits within the "-mfix-*" option family 
quite well though.

  Maciej

Re: MIPS atomic memory operations (A.K.A PR 33479).

[David Daney]

Maciej W. Rozycki wrote:
> On Wed, 19 Sep 2007, Ralf Baechle wrote:
> 
>> Please make this loop closure branch a branch-likely.  This is necessary
>> as a errata workaround for some processors.
> 
>  Do we emulate them for MIPS I?  We do emulate "ll" and "sc" and adding 
> "sync" is easy

Currently, I (and thus GCC 4.3) am assuming that Linux emulates 'll', 
'sc' and 'sync', If sync is not emulated, we would need to adjust the 
code generation so that it is not emitted on ISAs that don't support it.

> (as a no-op as support for R3000 SMP is unlikely to ever 
> happen).  Adding branches-likely, hmm...  Even though we do have logic to 
> do that as a part of the FP emulator.
> 
>  A workaround for a CPU erratum fits within the "-mfix-*" option family 
> quite well though.

Do we know which CPUs require branch-likely?

I would be inclined to agree with adding a "-mfix-??" option.

The only place where GCC's __sync_* primitives are generated without 
explicitly writing them into your program is in GCJ compiled java code 
that uses volatile fields.

If we expect the use of the __sync_* primitives on CPUs that require 
branch-likely to be rare, we shouldn't penalize those trying to rid 
themselves of the beasts.

> 
>   Maciej

Re: MIPS atomic memory operations (A.K.A PR 33479).

[Maciej W. Rozycki]

On Wed, 19 Sep 2007, David Daney wrote:

> Currently, I (and thus GCC 4.3) am assuming that Linux emulates 'll', 'sc' and
> 'sync', If sync is not emulated, we would need to adjust the code generation
> so that it is not emitted on ISAs that don't support it.

 While adding "sync" is trivial enough I may have a patch ready by 
tomorrow, that will not change the existing userbase and I am not entirely 
sure forcing such a hasty upgrade on people would be reasonable; likely 
not.

> > A workaround for a CPU erratum fits within the "-mfix-*" option family quite
> > well though.
> 
> Do we know which CPUs require branch-likely?

 The R10000; there is a note about it in <asm-mips/war.h> at 
R10000_LLSC_WAR.

> I would be inclined to agree with adding a "-mfix-??" option.
> 
> The only place where GCC's __sync_* primitives are generated without
> explicitly writing them into your program is in GCJ compiled java code that
> uses volatile fields.
> 
> If we expect the use of the __sync_* primitives on CPUs that require
> branch-likely to be rare, we shouldn't penalize those trying to rid themselves
> of the beasts.

 Another option is to depend on the setting of -mbranch-likely.  By 
default it is on only for the processors which implement it and do not 
discourage it, i.e. these of the MIPS II, MIPS III and MIPS IV ISAs.

  Maciej

Re: MIPS atomic memory operations (A.K.A PR 33479).

[David Daney]

Maciej W. Rozycki wrote:
> On Wed, 19 Sep 2007, David Daney wrote:
> 
>> Currently, I (and thus GCC 4.3) am assuming that Linux emulates 'll', 'sc' and
>> 'sync', If sync is not emulated, we would need to adjust the code generation
>> so that it is not emitted on ISAs that don't support it.
> 
>  While adding "sync" is trivial enough I may have a patch ready by 
> tomorrow, that will not change the existing userbase and I am not entirely 
> sure forcing such a hasty upgrade on people would be reasonable; likely 
> not.
> 
>>> A workaround for a CPU erratum fits within the "-mfix-*" option family quite
>>> well though.
>> Do we know which CPUs require branch-likely?
> 
>  The R10000; there is a note about it in <asm-mips/war.h> at 
> R10000_LLSC_WAR.
> 
>> I would be inclined to agree with adding a "-mfix-??" option.
>>
>> The only place where GCC's __sync_* primitives are generated without
>> explicitly writing them into your program is in GCJ compiled java code that
>> uses volatile fields.
>>
>> If we expect the use of the __sync_* primitives on CPUs that require
>> branch-likely to be rare, we shouldn't penalize those trying to rid themselves
>> of the beasts.
> 
>  Another option is to depend on the setting of -mbranch-likely.  By 
> default it is on only for the processors which implement it and do not 
> discourage it, i.e. these of the MIPS II, MIPS III and MIPS IV ISAs.

This seems to be the most sensible option.

I will try to work up the GCC patch tonight.

Thanks,
David Daney

Re: MIPS atomic memory operations (A.K.A PR 33479).

[Thiemo Seufer]

David Daney wrote:
> Maciej W. Rozycki wrote:
>> On Wed, 19 Sep 2007, David Daney wrote:
>>> Currently, I (and thus GCC 4.3) am assuming that Linux emulates 'll', 
>>> 'sc' and
>>> 'sync', If sync is not emulated, we would need to adjust the code 
>>> generation
>>> so that it is not emitted on ISAs that don't support it.
>>  While adding "sync" is trivial enough I may have a patch ready by 
>> tomorrow, that will not change the existing userbase and I am not entirely 
>> sure forcing such a hasty upgrade on people would be reasonable; likely 
>> not.
>>>> A workaround for a CPU erratum fits within the "-mfix-*" option family 
>>>> quite
>>>> well though.
>>> Do we know which CPUs require branch-likely?
>>  The R10000; there is a note about it in <asm-mips/war.h> at 
>> R10000_LLSC_WAR.
>>> I would be inclined to agree with adding a "-mfix-??" option.
>>>
>>> The only place where GCC's __sync_* primitives are generated without
>>> explicitly writing them into your program is in GCJ compiled java code 
>>> that
>>> uses volatile fields.
>>>
>>> If we expect the use of the __sync_* primitives on CPUs that require
>>> branch-likely to be rare, we shouldn't penalize those trying to rid 
>>> themselves
>>> of the beasts.
>>  Another option is to depend on the setting of -mbranch-likely.  By 
>> default it is on only for the processors which implement it and do not 
>> discourage it, i.e. these of the MIPS II, MIPS III and MIPS IV ISAs.
>
> This seems to be the most sensible option.
>
> I will try to work up the GCC patch tonight.

This means generic MIPS code (MIPS I) wil have broken atomic
intrinsics when run on modern MIPS machines.


Thiemo

Re: MIPS atomic memory operations (A.K.A PR 33479).

[Ralf Baechle]

On Wed, Sep 19, 2007 at 07:12:33PM +0100, Thiemo Seufer wrote:

> >>  Another option is to depend on the setting of -mbranch-likely.  By 
> >> default it is on only for the processors which implement it and do not 
> >> discourage it, i.e. these of the MIPS II, MIPS III and MIPS IV ISAs.

All MIPS implementations that have branch likely also support it with
good performance.  So the deprecation is atm really something that has
happened on paper.

The approach for LL/SC loops (where it's used for correctness) and the rest
of the code where we care about code size and performance is not necessarily
the same.

> > This seems to be the most sensible option.
> >
> > I will try to work up the GCC patch tonight.
> 
> This means generic MIPS code (MIPS I) wil have broken atomic
> intrinsics when run on modern MIPS machines.

Oh and if it takes adding new emulations for SYNC (some pseudo MIPS II
implementations lack SYNC afair) or branch likely to the kernel I will
certainly support that.

  Ralf

Re: MIPS atomic memory operations (A.K.A PR 33479).

[David Daney]

David Daney wrote:
> Maciej W. Rozycki wrote:
>> On Wed, 19 Sep 2007, Ralf Baechle wrote:
>>
>>> Please make this loop closure branch a branch-likely.  This is necessary
>>> as a errata workaround for some processors.
>>
>>  Do we emulate them for MIPS I?  We do emulate "ll" and "sc" and 
>> adding "sync" is easy
> 
> Currently, I (and thus GCC 4.3) am assuming that Linux emulates 'll', 
> 'sc' and 'sync', If sync is not emulated, we would need to adjust the 
> code generation so that it is not emitted on ISAs that don't support it.
> 

I just checked myself.  'sync' is not emulated.  We will have to make a 
change so that it is not emitted on ISAs that do not support it.

David Daney

Re: MIPS atomic memory operations (A.K.A PR 33479).

[Maciej W. Rozycki]

On Wed, 19 Sep 2007, David Daney wrote:

> I just checked myself.  'sync' is not emulated.  We will have to make a change
> so that it is not emitted on ISAs that do not support it.

 The problem is if such software is run on a newer processor it may 
silently break.  Tough, I know...  We should have added "sync" emulation 
long ago.  OTOH, perhaps the MIPS I userbase is not that large anymore for 
the emulation to be required with a short transition period only?

  Maciej