Need Alpha assembly implementation of the atomic routines for gcc

RESOLVED WONTFIX

Status

enhancement
P3
normal
RESOLVED WONTFIX
17 years ago
15 years ago

People

(Reporter: wtc, Assigned: wtc)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

17 years ago
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.
Assignee

Updated

17 years ago
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → Future

Comment 1

15 years ago
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.
Assignee

Comment 2

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

Updated

15 years ago
Attachment #165940 - Flags: review?(darin)

Comment 3

15 years ago
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

15 years ago
(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

15 years ago
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.
Attachment #165940 - Flags: review?(darin) → review+
Assignee

Comment 6

15 years ago
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.
Attachment #165940 - Attachment description: Patch for atomic operations in assembly on Alpha Linux → Patch for atomic operations in assembly on Alpha Linux (checked in)
Attachment #165940 - Flags: superreview+
Assignee

Comment 7

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

falk@debian.org, 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

15 years ago
(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).
Assignee

Comment 9

15 years ago
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

15 years ago
(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.
Assignee

Comment 11

15 years ago
falk@debian.org, I just changed "unsigned long" to
"PRInt32" in your patch.  Is this correct?
Attachment #165940 - Attachment is obsolete: true

Comment 12

15 years ago
(In reply to comment #11)
> Created an attachment (id=168066)
> Patch for atomic operations in assembly on Alpha Linux, v1.1
> 
> falk@debian.org, I just changed "unsigned long" to
> "PRInt32" in your patch.  Is this correct?

Yes, thank you.
Assignee

Updated

15 years ago
Attachment #168066 - Attachment description: Patch for atomic operations in assembly on Alpha Linux, v1.1 → Patch for atomic operations in assembly on Alpha Linux, v1.1 (checked in)
Assignee

Comment 13

15 years ago
I think it's unlikely anyone will ever work on
this.  Marked the bug WONTFIX.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → WONTFIX
Target Milestone: Future → 4.6
You need to log in before you can comment on or make changes to this bug.