151709 - Need Alpha assembly implementation of the atomic routines for gcc

Status: RESOLVED WONTFIX

(NSPR :: NSPR, enhancement, P3)

Description

[Wan-Teh Chang]

gcc cannot use the compiler intrinsics declared in
<machine/builtins.h> to implement NSPR atomic routines.
We need Alpha assembly implementation of _MD_ATOMIC_XXX.

Comment 1

[falk]

Attachment: Patch for atomic operations in assembly on Alpha Linux (checked in)

Hi,

this patch adds inline atomic operations for Alpha Linux. This gives a speedup
of around 30%, so it would be really nice if you could add it...

They should also be usable for Tru64, but I cannot test that.

Comment 2

[Wan-Teh Chang]

Thanks for the inline assembly code for Linux Alpha.
Darin, Bryner, would you like to review it?

Comment 3

[Darin Fisher]

So, existing code style suggests that these routines should be added to a file
named os_Linux_alpha.s in pr/src/md/unix/.  Any reason not to follow suit?

Comment 4

[falk]

(In reply to comment #3)
> So, existing code style suggests that these routines should be added to a file
> named os_Linux_alpha.s in pr/src/md/unix/.  Any reason not to follow suit?

Yes, these functions are very time-critical, and inlining them gave a measurable
speedup.

Comment 5

[Darin Fisher]

Comment on attachment 165940 [details] [diff] [review]
Patch for atomic operations in assembly on Alpha Linux (checked in)

r=darin

I don't vouch for the quality of the assembly in these macros (because I don't
know alpha assembly), but I do believe that you have used the proper defines,
and I agree with the way you have integrated this code into NSPR.  Perhaps we
should do the same for the other platforms.

Comment 6

[Wan-Teh Chang]

Comment on attachment 165940 [details] [diff] [review]
Patch for atomic operations in assembly on Alpha Linux (checked in)

I've checked in this patch on the NSPR trunk (NSPR 4.6)
and NSPRPUB_PRE_4_2_CLIENT_BRANCH (post Mozilla 1.8 Alpha 5 ).

I will leave this bug open until we have fixed this for
OSF1.

Comment 7

[Wan-Teh Chang]

Comment on attachment 165940 [details] [diff] [review]
Patch for atomic operations in assembly on Alpha Linux (checked in)

[e-mail], NSPR's atomic routines operate on
32-bit integers, but Alpha is a 64-bit processor.

Could you confirm that the assembly code in this
patch operates on 32-bit integers?  For example,
_MD_ATOMIC_ADD should have the following parameters
and return value:

PRInt32 _MD_ATOMIC_ADD(PRInt32 *ptr, PRInt32 val);

Comment 8

[falk]

(In reply to comment #7)
> Could you confirm that the assembly code in this
> patch operates on 32-bit integers?

Yes, they do, ldl_l and stl_c handle 32 bit, as opposed to ldq_l and stq_c.
I guess the temporary variables should also better be PRInt32, because then
gcc can omit the sign extension of the return value (although in practice, it
doesn't).

Comment 9

[Wan-Teh Chang]

Comment on attachment 165940 [details] [diff] [review]
Patch for atomic operations in assembly on Alpha Linux (checked in)

The reason I asked the question was exactly that
you declare the temporary variables as "unsigned long",
which is 64 bit.

Let me know if they should be changed to "int" or
"unsigned int".

Comment 10

[falk]

(In reply to comment #9)
> (From update of attachment 165940 [details] [diff] [review])
> The reason I asked the question was exactly that
> you declare the temporary variables as "unsigned long",
> which is 64 bit.
> 
> Let me know if they should be changed to "int" or
> "unsigned int".

Yes, please change them to "int", or "PRInt32", if you prefer.

Comment 11

[Wan-Teh Chang]

Attachment: Patch for atomic operations in assembly on Alpha Linux, v1.1 (checked in)

[e-mail], I just changed "unsigned long" to
"PRInt32" in your patch.  Is this correct?

Comment 12

[falk]

(In reply to comment #11)
> Created an attachment (id=168066)
> Patch for atomic operations in assembly on Alpha Linux, v1.1
> 
> [e-mail], I just changed "unsigned long" to
> "PRInt32" in your patch.  Is this correct?

Yes, thank you.

Comment 13

[Wan-Teh Chang]

I think it's unlikely anyone will ever work on
this.  Marked the bug WONTFIX.