502696 - js_CompareAndSwap on sparc should not use PRLock()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Wesley W. Garland]

I don't have the numbers to prove it, but my gut tells me that is probably a noticeable performance hit.

I have verifed that the Firefox 3.5 release candiate distributed on sunfreeware.com (why not mozilla.org?) by the Sun Beijing team suffers this defect. I originally spotted it in my local Tracemonkey-using embedding.

There is sparc assembly code in jslock.cpp which used to get included (back when it was jslock.c); I'm not sure what's happened in interm. I suspect this probably affects at least OpenSolaris as well; I'm not sure about sparc Linux.

Here's what js_CompareAndSwap looks like now in the firefox release candiate I downloaded:

(gdb) disassemble js_CompareAndSwap
Dump of assembler code for function js_CompareAndSwap:
0x00073f7c <js_CompareAndSwap+0>:       save  %sp, -96, %sp
0x00073f80 <js_CompareAndSwap+4>:       call  0x73f88 <js_CompareAndSwap+12>
0x00073f84 <js_CompareAndSwap+8>:       mov  %o7, %o7
0x00073f88 <js_CompareAndSwap+12>:      sethi  %hi(0xd8400), %o5
0x00073f8c <js_CompareAndSwap+16>:      sethi  %hi(0x9800), %i3
0x00073f90 <js_CompareAndSwap+20>:      add  %o5, 0x138, %o5
0x00073f94 <js_CompareAndSwap+24>:      xor  %i3, 0x58, %l7
0x00073f98 <js_CompareAndSwap+28>:      add  %o5, %o7, %l3
0x00073f9c <js_CompareAndSwap+32>:      mov  %i0, %l0
0x00073fa0 <js_CompareAndSwap+36>:      add  %l3, %l7, %l5
0x00073fa4 <js_CompareAndSwap+40>:      sethi  %hi(0x2c00), %i4
0x00073fa8 <js_CompareAndSwap+44>:      xor  %i4, 0x2dc, %l6
0x00073fac <js_CompareAndSwap+48>:      ld  [ %l5 ], %i0
0x00073fb0 <js_CompareAndSwap+52>:      add  %l3, %l6, %i4
0x00073fb4 <js_CompareAndSwap+56>:      cmp  %i0, 0
0x00073fb8 <js_CompareAndSwap+60>:      bne,pn   %icc, 0x73fdc <js_CompareAndSwap+96>
0x00073fbc <js_CompareAndSwap+64>:      nop
0x00073fc0 <js_CompareAndSwap+68>:      call  0x14c62c <PR_NewLock@plt>
0x00073fc4 <js_CompareAndSwap+72>:      sethi  %hi(0x9800), %l2
0x00073fc8 <js_CompareAndSwap+76>:      xor  %l2, 0x58, %o3
0x00073fcc <js_CompareAndSwap+80>:      st  %o0, [ %i4 ]
0x00073fd0 <js_CompareAndSwap+84>:      mov  1, %l1
0x00073fd4 <js_CompareAndSwap+88>:      add  %l3, %o3, %o2
0x00073fd8 <js_CompareAndSwap+92>:      st  %l1, [ %o2 ]
0x00073fdc <js_CompareAndSwap+96>:      call  0x14c680 <PR_Lock@plt>
0x00073fe0 <js_CompareAndSwap+100>:     ld  [ %i4 ], %o0
0x00073fe4 <js_CompareAndSwap+104>:     ld  [ %l0 ], %i5
0x00073fe8 <js_CompareAndSwap+108>:     xor  %i5, %i1, %i1
0x00073fec <js_CompareAndSwap+112>:     srl  %i1, 0, %o7
0x00073ff0 <js_CompareAndSwap+116>:     neg  %o7, %o4
0x00073ff4 <js_CompareAndSwap+120>:     srlx  %o4, 0x3f, %l4
0x00073ff8 <js_CompareAndSwap+124>:     xorcc  %l4, 1, %i0
0x00073ffc <js_CompareAndSwap+128>:     bne,a   %icc, 0x74004 <js_CompareAndSwap+136>
0x00074000 <js_CompareAndSwap+132>:     st  %i2, [ %l0 ]
0x00074004 <js_CompareAndSwap+136>:     call  0x14c698 <PR_Unlock@plt>
0x00074008 <js_CompareAndSwap+140>:     ld  [ %i4 ], %o0
0x0007400c <js_CompareAndSwap+144>:     ret
0x00074010 <js_CompareAndSwap+148>:     restore
End of assembler dump.

Comment 1

[Wesley W. Garland]

Ginn Chen: Adding you to CC list because I think you're the person building the browsers uploaded to sunfreeware.com.   Are you specifying --enable-js-ultrasparc in your ./configure?

Comment 2

[Wesley W. Garland]

Attachment: Patch to fix js_CompareAndSwap and deprecate sparc v8

The root of this bug boils to:
  --enable-js-ultrasparc option in configure.in never does anything, because the target is always 'sparc' and configure.in doesn't trust the user and double-checks to make sure it is 'sun4u' (following up with a silent override).

This patch fixes js_CompareAndSwap, yielding the following on an optimized build:

(gdb) disassemble js_CompareAndSwap
Dump of assembler code for function js_CompareAndSwap:
0xff27aa6c <js_CompareAndSwap+0>:       stbar
0xff27aa70 <js_CompareAndSwap+4>:       unknown
0xff27aa74 <js_CompareAndSwap+8>:       cmp  %o1, %o2
0xff27aa78 <js_CompareAndSwap+12>:      be,a   0xff27aa84 <js_CompareAndSwap+24>
0xff27aa7c <js_CompareAndSwap+16>:      mov  1, %o2
0xff27aa80 <js_CompareAndSwap+20>:      clr  %o2
0xff27aa84 <js_CompareAndSwap+24>:      cmp  %g0, %o2
0xff27aa88 <js_CompareAndSwap+28>:      retl
0xff27aa8c <js_CompareAndSwap+32>:      addx  %g0, 0, %o0
End of assembler dump.

This patch also removes the --enable-js-ultrasparc configuration switch from both the top-level and js/src/configure.in files -- it was used only to select whether v8 or v9 instructions were used in lock_SunOS.s  (which should have been called lock_sparc.s).  However, the v8 code was not imported into jslock.cpp when that file was deprecated (this is at about the same time x86_64 support was added).

If anybody is concerned about removing sparc v8 support -- the nanojit uses v9 instructions, the fastest v8 CPU was 200MHz, v8 was dropped from Solaris 10 (released in 2003), the newest v8 CPU was made in 1996, and building with Sun Studio already forces v9 instructions via the -xarch=v8plus switch.

Also, ted suggests that 'v8' is overloaded in jsapi context and that that alone is a good enough reason to drop support for sparc v8 instructions. ;)

I also made a couple of other drive-by corrections:
 - Use GCC syntax to select PIC and v9 instructions; previous versions are likely to emit v7 code (yes, so you could theoretically run it on a box built in 1985) -- also, previous versions pass -K PIC to the assembler with -Wa, hoping that GCC was built to use /usr/ccs/bin/as rather than gas

 - No longer assert that ov != nv in NativeCompareAndSwap() for sparc. No other platform does this, and I see no reason why sparc should.

 - Now that ULTRA_SPARC is no longer a testable, compare_and_lock() will get called for Sun Studio builds, rather than locking with PRLock().  It's possible that this inlinable NativeCompareAndSwap() will be faster, but changing that is above my pay grade. (Ginn?)

 - No longer guard for solaris; we just check compiler and processor. This will hopefully let sparc Linux use this patch, providing it's built with GCC.

Incidentally, if hg has the concept of a cvs-attic, lock_SunOS.s belongs there. It's unused as-of autoconf and the only "good stuff" left in it is a version of js_CompareAndSwap that will run on v8 CPUs.

*** Warning to Solaris 10 Users *** -- this patch, by virtue of emitting v9 instructions, triggers a bug in ancient versions of the GNU assembler, such as the one that ships in /usr/sfw/ with Solaris 10 and early versions of Nevada. If you get this error: 

{standard input}: Assembler messages:
{standard input}:2276: Error: Illegal operands: There are only 32 single precision f registers; [0-31]
make[2]: *** [jsdtoa.o] Error 1

you are affected. This is not Mozilla's bug, it is GNU's and the fix is to upgrade to a reasonably current (say, only five years old) version of gas. My suggestion is to install the GNU binutils package from sunfreeware.com, and link it to
/usr/sfw/libexec/gcc/sparc-sun-solaris2.10/3.4.3/as
/usr/sfw/sparc-sun-solaris2.10/bin/as
/usr/sfw/bin/gas

Comment 3

[Ginn Chen]

First, I didn't upload builds to sunfreeware.com.
Alfred Peng contributes builds to mozilla.org but not sunfreeware.com.
I believe he didn't use --enable-js-ultrasparc.

Second, there're some problems with your patch.
You didn't remove #ifndef ULTRA_SPARC from lock_SunOS.s.
jslock is a C++ program now, you need to move extern int compare_and_swap(jsword*, jsword, jsword); out of NativeCompareAndSwap(), and use extern "C".  Otherwise it won't link.

Third, the minimal requirement of compiler is Sun Studio 12 now, and it supports gcc inline asm.
So I think we can just remove " #if defined(__GNUC__)" from NativeCompareAndSwap() and remove lock_SunOS.s from Makefile.in. 
We can do the same for x86.

Thanks!

Comment 4

[Wesley W. Garland]

Ginn, thanks for the follow up!

I know this is the wrong forum -- but do you know where those packages on sunfreeware.com originate from? The page says "Internal Sun Developers".

Do you know if Alfred has had time to contribute a Firefox 3.5 build at mozilla.org yet? I've looked, and can't find one.  Incidentally, the sunfreeware.com package I installed was quite good, the package stream had packages for pango, cairo, and a number of other dependencies, making installation a breeze.

Thanks for the looking over that patch. I'm also going to try and get it tested on Linux for SPARC before requesting code review.

Re your comments:
 - No need to remove ULTRA_SPARC from lock_SunOS.s, that file should be removed from the repository
 - Do you think we should replace compare_and_swap() with the inline-assembly? I do, but didn't know if there was some special Sun Studio magic I didn't know about
 - Thanks for the note on the Sun Studio inline assembler syntax, I wasn't aware of that

Comment 5

[Wesley W. Garland]

Attachment: Patch to fix js_CompareAndSwap and deprecate sparc v8

This patch 
 - removes lock_SunOS.s from the repo
 - removes #ifdef SOLARIS dependency on NativeCompareAndSwap implementation, now checking only for sparc
 - removes SunStudio compare_and_swap() code so both GCC and Sun CC run the same code

Ginn, would you mind reviewing this patch?  Jorendorff (js peer) says "Wes--: get someone from Sun to review it first and I'll stamp an r+ on it and push it in"

Note: I believe this patch works on Linux and with Sun Studio, but have only tested GCC on Solaris 10

Comment 6

[Ginn Chen]

(In reply to comment #4)
> Ginn, thanks for the follow up!
> 
> I know this is the wrong forum -- but do you know where those packages on
> sunfreeware.com originate from? The page says "Internal Sun Developers".
I don't know.
> 
> Do you know if Alfred has had time to contribute a Firefox 3.5 build at
> mozilla.org yet? I've looked, and can't find one.  Incidentally, the
> sunfreeware.com package I installed was quite good, the package stream had
> packages for pango, cairo, and a number of other dependencies, making
> installation a breeze.
You can find the builds from the link at
http://opensolaris.org/os/community/desktop/communities/mozilla/development/

> 
> Thanks for the looking over that patch. I'm also going to try and get it tested
> on Linux for SPARC before requesting code review.
> 
> Re your comments:
>  - No need to remove ULTRA_SPARC from lock_SunOS.s, that file should be removed
> from the repository
That's right.

>  - Do you think we should replace compare_and_swap() with the inline-assembly?
Yes.

> I do, but didn't know if there was some special Sun Studio magic I didn't know
> about
Nothing special, but it should be tested.

Your patch looks good to me.
Comments:
1) You should remove ASFILES = lock_$(OS_ARCH).s in Makefile.in
2) For x86, can you change if defined(__GNUC__) to if defined(__GNUC__) || defined (__SUNPRO_CC), or do you want to do it in a separate bug?

I'm out of office today. I'll give it a test tomorrow.
It should be no problem, I did a quick test yesterday.

Comment 7

[Wesley W. Garland]

Attachment: Patch to fix js_CompareAndSwap and deprecate sparc v8

Addresses issues raised in Ginn's last comment, including making inline assembly available to Sun Studio for i386 and x64_64 platforms.

Comment 8

[Wesley W. Garland]

Attachment: Patch to fix js_CompareAndSwap and deprecate sparc v8

Missed corresponding changes to jslock.h

Comment 9

[Ginn Chen]

Attachment: Patch to fix js_CompareAndSwap and deprecate sparc v8

Almost the same patch.
There're some small differences between Sun Studio and gcc.
Sun Studio has __i386 not __i386__.
Sun Studio doesn't recognize __asm__ and __volatile__.
I fixed them and verified on Solaris SPARC, x86, x86_64.

Comment 10

[Wesley W. Garland]

Attachment: Patch to fix js_CompareAndSwap and deprecate sparc v8

This is the same patch Ginn uploaded, except for two minor details:
 - lock_SunOS.s hg rm'd
 - accidental xpconnect changes from another bug removed

Comment 11

[Ginn Chen]

Thanks.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 388554 [details] [diff] [review]
Patch to fix js_CompareAndSwap and deprecate sparc v8

`asm volatile` works in GCC. Please combine the GCC and SUNPRO_CC cases.

If you wanted to change the sparc asm statement to look like this:

    asm volatile ("stbar\n"
                  "cas [%1],%2,%3\n"
                  ...)

I wouldn't object!

>-ifdef JS_ULTRASPARC_OPTS
>-DEFINES += -DULTRA_SPARC
> ifdef GNU_CC
>-CFLAGS   += >-Wa,-xarch=v8plus,-DULTRA_SPARC,-P,-L,-D_ASM,-D__STDC__=0
>-CXXFLAGS += -Wa,-xarch=v8plus,-DULTRA_SPARC,-P,-L,-D_ASM,-D__STDC__=0,-K,PIC
>+CFLAGS   += -mcpu=v9
>+CXXFLAGS += -mcpu=v9

It looks like this will turn on -mcpu=v9 for gcc on all platforms!

>+ASFLAGS  += -xarch=v8plus -P -L -D_ASM -D__STDC__=0 -K PIC

Since you're dropping v8 support here, you could bump -xarch=v8plus to (I guess) -xarch=v9.

It's too bad there's not a macro that says what version of sparc you're compiling for.  I see `__sparcv9`, but it comes with the caveat "64-bit compilation modes only". Oh well.

Comment 13

[Jason Orendorff [:jorendorff]]

(In reply to comment #12)
> >-ifdef JS_ULTRASPARC_OPTS
> >-DEFINES += -DULTRA_SPARC
> > ifdef GNU_CC
> >-CFLAGS   += >-Wa,-xarch=v8plus,-DULTRA_SPARC,-P,-L,-D_ASM,-D__STDC__=0
> >-CXXFLAGS += -Wa,-xarch=v8plus,-DULTRA_SPARC,-P,-L,-D_ASM,-D__STDC__=0,-K,PIC
> >+CFLAGS   += -mcpu=v9
> >+CXXFLAGS += -mcpu=v9
> 
> It looks like this will turn on -mcpu=v9 for gcc on all platforms!

I'm wrong, of course. there's an ifdef(sparc) above that.

Comment 14

[Jason Orendorff [:jorendorff]]

This change will make it impossible to build Firefox on SPARC v8.  Wes, could you please post to the mozilla.dev.builds newsgroup about this?  Just fair warning and a link to this bug would suffice.  It might also be worth pointing out just how old SPARC v8 is.

Comment 15

[Wesley W. Garland]

 - newsgroup comment posted: http://groups.google.ca/group/mozilla.dev.builds/browse_thread/thread/95b60801d79ae1e9#

 - -xarch=v8plus is actually the right flag, believe it or not, it means "use v9 instructions and a 32-bit build"

 - lack of appropriate detection mechanism is one major driver behind deprecating v8, as you point out compiler flags aren't useful.

Comment 16

[Wesley W. Garland]

Attachment: Patch to fix js_CompareAndSwap and deprecate sparc v8

This patch should not introduce any functional changes from the above, but addresses style/consistency problems and eliminates duplicate code.

Observation: GCC and SunStudio share a common inline-assembler syntax and some common machine definition macros. However, SunStudio supports a subset of what GCC supports, and the jsapi code-base is a bit mixy-matchy here -- possibly room for formal rules down the road.

This is what I've done in jslock.cpp and jslock.h:

 - __asm__ and __volatile__ always in use in the code body, but macroed to asm and volatile when not __GNUC__ (suggested practice in GCC docs)

 - use __x86_64, __sparc, and __i386 as machine-type determiners, rather than __x86_64__, sparc and __i386__

FWIW, a quick way to check what symbols your platform defines with gcc:
# gcc -E -dM - < /dev/null | egrep 'sparc|i386|x86_64'

Neither of these rules apply when _WIN32 && _M_IX86 -- thats a roundabout way of saying "Windows, x86 and Microsoft or Intel Compiler".   It appears that ICC on non-Windows will fall through to PRLock, but that's the case without this patch, too.

Comment 17

[Ginn Chen]

http://hg.mozilla.org/mozilla-central/rev/566ec4a87812

Comment 18

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-CC) Copy it to comm-central [Checkin: Comment 21]

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Clean up missed autoconf.mk.in too [Checkin: Comment 20]

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 428007 [details] [diff] [review]
(Cv1) Clean up missed autoconf.mk.in too
[Checkin: Comment 20]


http://hg.mozilla.org/mozilla-central/rev/d768ae4fd568

Comment 21

[Serge Gautherie (:sgautherie)]

Comment on attachment 428006 [details] [diff] [review]
(Bv1-CC) Copy it to comm-central
[Checkin: Comment 21]


http://hg.mozilla.org/comm-central/rev/0a20fe4f2aed