502369 - sparc linux lacks sync_instruction_memory rendering xulrunner unbuildable

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Dennis Gilmore]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b4) Gecko/20090427 Fedora/3.5-0.20.beta4.fc11 Firefox/3.5b4
Build Identifier: 

sparclinux libc does not have sync_instruction_memory  the attached patch add support for sparc linux  so it will again be buildable. 

Reproducible: Always

Steps to Reproduce:
1. compile 
2.
3.
Actual Results:  
build fails with missing symbol

Expected Results:  
compilation to succeed

Comment 1

[Dennis Gilmore]

Attachment: patch to add sync_instruction_memory

Comment 2

[Reed Loden [:reed]]

Not sure if this is the right component or not...

Comment 3

[Andreas Gal :gal]

Comment on attachment 386822 [details] [diff] [review]
patch to add sync_instruction_memory

Ed, you guys might want this too.

Comment 4

[Edwin Smith]

Comment on attachment 386822 [details] [diff] [review]
patch to add sync_instruction_memory

added tamarin bug 502642 to track this one

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 386822 [details] [diff] [review]
patch to add sync_instruction_memory

>+		asm("flush %0" : : "r" (p));

IMHO, this should be made volatile. And iflush might be more appropriate than flush, if I'm not mistaken.

Comment 7

[Leon Sha]

Someone mentioned here using iflush
https://bugzilla.mozilla.org/show_bug.cgi?id=486584#c2
And in solaris code flush is used.
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/lib/libc/sparc/gen/sync_instruction_memory.s
I think both is OK.

>+		p += 32;
I don't understand why it is 32 here. Should be 8.

Comment 8

[Lars T Hansen]

(In reply to comment #6)
> (From update of attachment 386822 [details] [diff] [review])
> >+		asm("flush %0" : : "r" (p));
> 
> IMHO, this should be made volatile. And iflush might be more appropriate than
> flush, if I'm not mistaken.

In the Sparc v8 and v9 manuals it is called "flush"; it was called "iflush" in v7.  Presumably the old name lives on in various assemblers.

Comment 9

[David S. Miller]

Nobody will ever make a sparc V9 chip with an I-cache with line sizes
smaller than 32-bytes.

Doing it 8 bytes at a time is simply a bunch of wasted full pipeline flushes.

That's why we iterate 32-bytes at a time.

Comment 10

[Mike Hommey [:glandium]]

(In reply to comment #9)
> Nobody will ever make a sparc V9 chip with an I-cache with line sizes
> smaller than 32-bytes.

Is spidermonkey targetted at v9+ only, now ?

Comment 11

[David S. Miller]

If you find even one person running this code on a V8 or earlier
Sparc processor, I will buy you a multi-million dollar penthouse
condo overlooking Central Park in New York City.

Comment 12

[Robert Accettura [:raccettura]]

(In reply to comment #11)
> If you find even one person running this code on a V8 or earlier
> Sparc processor, I will buy you a multi-million dollar penthouse
> condo overlooking Central Park in New York City.

Well this machine is only $49.

http://cgi.ebay.com/Sun-SPARCstation-5-96MB-microSPARC-II-CD-ROM-FDD_W0QQitemZ390063492867QQcmdZViewItemQQptZLH_DefaultDomain_0?hash=item5ad1988f03&_trksid=p3286.c0.m14&_trkparms=65%3A12|66%3A2|39%3A1|72%3A1234|293%3A1|294%3A50

Will you cover moving costs?

Comment 13

[Wesley W. Garland]

@Mike & David: I have proposed deprecating SPARC V8 support entirely as part of the fix for bug 502696.

@Robert: The fastest SPARCstation 5 ever made ran at 170MHz and the 170MHz doesn't even run on Linux as it doesn't support the Fujitsu TurboSPARC (for whatever reason).

To put this in perspective, I tried to today to run Firefox on a multi-core 400MHz UltraSparc-II system (significantly faster clock rate and 3 times as many instructions per cycle).  Technically speaking, it was usable, but 30 seconds to render "google.com" is not encouraging.

Besides which, NativeSparc.cpp already emits V9 instructions. So assuming that the CPU in use is an ultrasparc is completely safe.

Comment 14

[Edwin Smith]

Is this patch still applicable to newly merged nanojit in tamarin-redux?

Comment 15

[Edwin Smith]

status anyone?

Comment 17

[Dennis Gilmore]

Can someone please apply this patch

Comment 18

[Lars T Hansen]

I'll take the action to land (if appropriate) or close (otherwise), though I do not have access to a sparc-linux system so I don't know how much testing I can really hope to do.

(Leon, feel free to take this back from me if you want.)

Comment 19

[Lars T Hansen]

pushed: redux changeset:   2929:a27726496703

Comment 20

[Chris Peyer]

Engineering work item.  Marking as verified.

Comment 21

[Raúl Porcel]

This is needed on mozilla-1.9.2. Can we get this backported, please?

Thanks

Comment 22

[Lars T Hansen]

No longer a tamarin issue but a Mozilla nanojit issue.

Comment 23

[Andreas Gal :gal]

reed, this is not part of the build. We don't ship on solaris. We should just land this imo.

Comment 24

[Mike Hommey [:glandium]]

(In reply to comment #23)
> reed, this is not part of the build. We don't ship on solaris. We should just
> land this imo.

just a nit: this is not a patch for solaris, but for linux on sparc processors.

Comment 25

[Reed Loden [:reed]]

(In reply to comment #23)
> reed, this is not part of the build. We don't ship on solaris. We should just
> land this imo.

In general, approval is still required for landing on branches.

Comment 26

[Andreas Gal :gal]

Rules and reality don't have to be mutually exclusive. We don't ship on linux (vendors do), even less so do we ship sparc, and the total number of linux sparc firefox users is probably between 2 and 5. But I guess it can't hurt to get approval (that's why I didn't touch your flag), so lets just wait until one of the drivers wanders by.

Comment 27

[Reed Loden [:reed]]

(In reply to comment #26)
> We don't ship on linux (vendors do)

We most definitely ship on Linux. I only use mozilla.org builds, for one example.

Comment 28

[Brandon Sterne (:bsterne)]

Comment on attachment 386822 [details] [diff] [review]
patch to add sync_instruction_memory

We missed 1.9.2.2.  Moving approval request forward.

Comment 29

[Mike Hommey [:glandium]]

Attachment: Patch for 1.9.2

The code moved in 1.9.2, and is not in Assembler.cpp anymore. If that is going to land on 1.9.2, this is the patch.

Comment 30

[Raúl Porcel]

Any news?

C'mon...its an easy fix

Comment 32

[Raúl Porcel]

Another one bites the dust!

Comment 33

[Mike Hommey [:glandium]]

Comment on attachment 435828 [details] [diff] [review]
Patch for 1.9.2

Let's try again for next round

Comment 34

[LegNeato]

Comment on attachment 435828 [details] [diff] [review]
Patch for 1.9.2

a=LegNeato for 1.9.2.12. Please land only on the mozilla-1.9.2 default branch, *not* the relbranch.

Comment 35

[Mike Hommey [:glandium]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7d8873ec9851