Closed
Bug 504202
Opened 15 years ago
Closed 15 years ago
MIPS target support
Categories
(Tamarin Graveyard :: Virtual Machine, defect, P2)
Tracking
(Not tracked)
VERIFIED
FIXED
flash10.1
People
(Reporter: chris, Assigned: rreitmai)
References
Details
(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)
Attachments
(5 files, 9 obsolete files)
(deleted),
patch
|
rreitmai
:
review+
edwsmith
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rreitmai
:
review+
brbaker
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
application/octet-stream
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.11) Gecko/2009060309 Ubuntu/9.04 (jaunty) Firefox/3.0.11
Build Identifier:
The attached patch adds build support for the MIPS target.
At present there is no JIT support for MIPS, but it is possible to build the interpreter.
Reproducible: Always
After prebuilding all of the .abc files on an x86 host, the MIPS interpreter passes all of the acceptance tests except:
abcasm/syntaxErrors.abs: My test system does not have a native java compiler installed
ecma3/String/e15_5_4_7_2.as: I've tracked this down to core/StringObject.cpp where it casts an out of range float value to an integer which generates an undefined value. On MIPS the undefined value is positive, on x86 it is negative.
I'll file a separate bug report for this.
Reporter | ||
Comment 1•15 years ago
|
||
Comment 2•15 years ago
|
||
Comment on attachment 388588 [details] [diff] [review]
Enable MIPS support
Looks good.
Since MMGC_MIPS and AVMPLUS_MIPS are not used in any of the code at present, there should be no need to introduce them in avmfeatures.as.
Comment 3•15 years ago
|
||
("Build Config" component is more for the test harness etc; makefiles and configuration are part of the vm component.)
Status: UNCONFIRMED → NEW
Component: Build Config → Virtual Machine
Ever confirmed: true
QA Contact: build-config → vm
Reporter | ||
Comment 4•15 years ago
|
||
2:
AVMPLUS_MIPS was being used in platform/unix/unixcpuid.cpp to select an
implementation of P4Available() that didn't involve x86 assembler. The only user of P4Available() is shell/ShellCore.cpp iff JIT && (IA32 || AMD64), so there is really no need for P4Available() to be defined at all.
I've split this patch into 2 parts. Part 1 restricts P4Available() so that it is only defined and used for targets that need it (IA32 & AMD64). Part 2 contains the MIPS changes without MMGC_MIPS and AVMPLUS_MIPS defined.
Strictly speaking Part1 is no longer part of the MIPS target support. If you prefer, I could file the P4Available change as a separate bug and make this one dependent on it?
Comment 5•15 years ago
|
||
separate bug isn't required, you can just attach the separate patches here
Reporter | ||
Comment 6•15 years ago
|
||
Removed unused legacy definitions
Attachment #388588 -
Attachment is obsolete: true
Reporter | ||
Comment 7•15 years ago
|
||
Updated•15 years ago
|
Attachment #388961 -
Flags: review?(rreitmai)
Updated•15 years ago
|
Attachment #388962 -
Flags: review?(rreitmai)
Assignee | ||
Comment 8•15 years ago
|
||
Comment on attachment 388961 [details] [diff] [review]
Enable MIPS support
+'ing but should runtests.py use lower case - 'mips' not 'MIPS'
Attachment #388961 -
Flags: review?(rreitmai) → review+
Assignee | ||
Updated•15 years ago
|
Attachment #388962 -
Flags: review?(rreitmai) → review+
Reporter | ||
Comment 9•15 years ago
|
||
(In reply to comment #8)
> (From update of attachment 388961 [details] [diff] [review])
> +'ing but should runtests.py use lower case - 'mips' not 'MIPS'
file(1) returns MIPS in uppercase for most formats. The exceptions are Mach-O and NetBSD. I can change this to
elif re.search('(mips)', f.lower()):
to catch those too.
Incidentally I noticed a problem with the way this detection works. You can get a false positive from the file/directory name which file(1) prints out by default. Eg passing --avm=/opt/appcore/tamarin-redux/objdir-mips/shell/avmshell would convince the script it was running on ppc(!). Modern versions of file(1) have a -b option to suppress the filename output, but for portability there should be something like this in runtestBase.py:
f = ' '.join(f)
+ f = f.replace(self.avm, '');
self.verbose_print('determineConfig: %s' % f)
Comment 10•15 years ago
|
||
Tracking update for config detection in runtests.py in comment #9 as bug #512934
Comment 11•15 years ago
|
||
Rick, you approved a couple of these patches, are they ready to land? Can you land them if you have time?
Assignee | ||
Comment 12•15 years ago
|
||
ok, will look into landing them next week.
Updated•15 years ago
|
Assignee: nobody → rreitmai
Priority: -- → P3
Target Milestone: --- → flash10.1
Reporter | ||
Comment 13•15 years ago
|
||
Ignore case when checking binary type.
Refreshed to head of tree
Reporter | ||
Comment 14•15 years ago
|
||
Basic JIT support for MIPS
Reporter | ||
Updated•15 years ago
|
Attachment #388961 -
Attachment is obsolete: true
Updated•15 years ago
|
Priority: P3 → P4
Comment 15•15 years ago
|
||
Raising priority for the MIPS implementation to a must-have for 10.1. Rick is on the hook for review in the next week or so.
Flags: flashplayer-qrb+
Priority: P4 → P2
Reporter | ||
Updated•15 years ago
|
Attachment #404635 -
Flags: review?(rreitmai)
Assignee | ||
Comment 16•15 years ago
|
||
The patch looks good so far (I'll do a more extensive review tomorrow or early next week), but I assume that we don't want to land it unless we have a h/w platform as part of our nightly builds or at least until we can run it on something.
Reporter | ||
Comment 17•15 years ago
|
||
Attachment #405736 -
Flags: review?(rreitmai)
Reporter | ||
Comment 18•15 years ago
|
||
Attachment #405737 -
Flags: review?(rreitmai)
Assignee | ||
Updated•15 years ago
|
Attachment #405737 -
Flags: review?(rreitmai) → review+
Assignee | ||
Updated•15 years ago
|
Attachment #405736 -
Flags: review?(rreitmai) → review+
Assignee | ||
Comment 19•15 years ago
|
||
Comment on attachment 388962 [details] [diff] [review]
P4Available is only needed for IA32 and AMD64 JIT builds
P4Avail change can be landed now.
Attachment #388962 -
Flags: superreview?(edwsmith)
Updated•15 years ago
|
Attachment #388962 -
Flags: superreview?(edwsmith) → superreview+
Assignee | ||
Updated•15 years ago
|
Attachment #404632 -
Flags: review?(brbaker)
Comment 20•15 years ago
|
||
Comment on attachment 404632 [details] [diff] [review]
Update: Enable MIPS support
Also adding an additional patch to enable:
../configure.py --enable-shell --target=mips-linux
Attachment #404632 -
Flags: review?(brbaker) → review+
Comment 21•15 years ago
|
||
Apply after Attachment 404632 [details] [diff], this enables:
../configure.py --enable-shell --target=mips-linux
Need to either link gcc, g++, ar to MIPS tool chain or export the following
CXX=mips-linux-gnu-g++
CC=mips-linux-gnu-gcc
LD=mips-linux-gnu-ar
AR=mips-linux-gnu-ar
There is a hack involved with the current patch to enable the same switch combination as gcc 4.2 since there are currently too many errors to be able to compile with -Werror
Attachment #409763 -
Flags: review?(rreitmai)
Assignee | ||
Updated•15 years ago
|
Attachment #409763 -
Flags: review?(rreitmai) → review+
Assignee | ||
Comment 22•15 years ago
|
||
Reporter | ||
Comment 23•15 years ago
|
||
This patch does not include a JIT. Use the --enable-wordcode-interp
option when configuring.
Attachment #404632 -
Attachment is obsolete: true
Attachment #410410 -
Flags: review?(rreitmai)
Reporter | ||
Comment 24•15 years ago
|
||
Nanojit support for MIPS.
This patch only modifies the nanojit directory
It combines all of the previous JIT related patches and adds the following:
updated to head of tree (2977:4c213156e2d5)
bugfix for passing mixed float/integer arguments
suppressed unused argument warnings
cosmetic (whitespace & stylistic) changes
It does not enable JIT support
Attachment #404635 -
Attachment is obsolete: true
Attachment #405736 -
Attachment is obsolete: true
Attachment #405737 -
Attachment is obsolete: true
Attachment #410414 -
Flags: review?(rreitmai)
Attachment #404635 -
Flags: review?(rreitmai)
Reporter | ||
Comment 25•15 years ago
|
||
This patch enables nanojit support for MIPS
Reporter | ||
Updated•15 years ago
|
Attachment #410416 -
Flags: review?(rreitmai)
Reporter | ||
Comment 26•15 years ago
|
||
(In reply to comment #21)
> There is a hack involved with the current patch to enable the same switch
> combination as gcc 4.2 since there are currently too many errors to be able to
> compile with -Werror
I've posted a bugreport for some alignment warnings
https://bugzilla.mozilla.org/show_bug.cgi?id=526666
With these patches in place the code compiles cleanly with the CS gcc-4.3-154 compiler
Assignee | ||
Comment 27•15 years ago
|
||
Comment on attachment 410414 [details] [diff] [review]
Nanojit support for MIPS
asking graydon to review the NJ specific for landing in nanojit-central
Attachment #410414 -
Flags: review?(graydon)
Assignee | ||
Comment 28•15 years ago
|
||
Comment on attachment 409763 [details] [diff] [review]
MIPS x-platform compile patch
Looks good to me...asking Brent to review (and land?) configure patches.
Attachment #409763 -
Flags: review?(brbaker)
Assignee | ||
Updated•15 years ago
|
Attachment #410416 -
Flags: review?(rreitmai)
Attachment #410416 -
Flags: review?(brbaker)
Attachment #410416 -
Flags: review+
Comment 29•15 years ago
|
||
Comment on attachment 409763 [details] [diff] [review]
MIPS x-platform compile patch
Pushed changeset: 3007:c35559a0d468
Attachment #409763 -
Attachment is obsolete: true
Attachment #409763 -
Flags: review?(brbaker)
Comment 30•15 years ago
|
||
Comment on attachment 410410 [details] [diff] [review]
Add MIPS target to configuration/build system
Pushed changeset: 3007:c35559a0d468
Attachment #410410 -
Flags: review?(rreitmai) → review+
Updated•15 years ago
|
Attachment #410410 -
Attachment is obsolete: true
Comment 31•15 years ago
|
||
Comment on attachment 410416 [details] [diff] [review]
Build nanojit for MIPS targets
Pushed changeset: 3007:c35559a0d468
Attachment #410416 -
Flags: review?(brbaker) → review+
Assignee | ||
Updated•15 years ago
|
Attachment #410414 -
Flags: review?(rreitmai) → review+
Comment 32•15 years ago
|
||
Comment on attachment 410414 [details] [diff] [review]
Nanojit support for MIPS
Brief comments:
- There's no support here for configure.in to notice we're targeting MIPS and define AVMPLUS_MIPS and NANOJIT_ARCH=MIPS; patch-as-proposed won't build out of nanojit-central.
- Predicate on AVMPLUS_MIPS in nanojit.h, not AVMSYSTEM_PLUS.
- As with all the other mode-selections, hard-wiring to LE/BE mode with CPP symbols seems undesirable. Would prefer a runtime flag.
- The const flags (fpu, cmov, etc.) should similarly be runtime flags in the AVM config. Generally we want to make it possible to build 1 binary for a given arch that runs on a reasonable set of variants by auto-detecting capabilities during startup.
- Hard tabs! Verboten in nanojit. Spaces only, 4-space indent.
Otherwise, well, I don't really know MIPS assembly at all; it looks a bit incomplete and/or broken in some logic, but it's not like we have anything better at the moment. Good first step! May as well land once the above are fixed.
Attachment #410414 -
Flags: review?(graydon) → review+
Reporter | ||
Comment 33•15 years ago
|
||
(In reply to comment #32)
> (From update of attachment 410414 [details] [diff] [review])
> Brief comments:
>
> - There's no support here for configure.in to notice we're targeting MIPS and
> define AVMPLUS_MIPS and NANOJIT_ARCH=MIPS; patch-as-proposed won't build out of
> nanojit-central.
The final patch was submitted just as the changeover was happening. I've checked out the nanojit-central tree. Should I resubmit the JIT patch against that tree?
> - Predicate on AVMPLUS_MIPS in nanojit.h, not AVMSYSTEM_PLUS.
OK. The AVMPLUS_{cpu} flags are marked as legacy names in core/avmfeatures.as so I assumed they were deprecated.
> - As with all the other mode-selections, hard-wiring to LE/BE mode with CPP
> symbols seems undesirable. Would prefer a runtime flag.
Hmm... I thought that 64 bit values were stored in native format and the JIT needs to use the same ordering so I'm not clear why this decision should be made at runtime?
> - The const flags (fpu, cmov, etc.) should similarly be runtime flags in the
> AVM config. Generally we want to make it possible to build 1 binary for a given
> arch that runs on a reasonable set of variants by auto-detecting capabilities
> during startup.
I'll define the cpu_has_* flags as variables. I assume it's ok to preinitialise the values based on the compilation flags? The rationale is that compiler is generating code based on the same information so the JIT should use the same defaults. If some external code wants to override those choices it would be free to do so.
> - Hard tabs! Verboten in nanojit. Spaces only, 4-space indent.
Understood. I'll reformat the code.
>
> Otherwise, well, I don't really know MIPS assembly at all; it looks a bit
> incomplete and/or broken in some logic, but it's not like we have anything
> better at the moment. Good first step! May as well land once the above are
> fixed.
Thanks for taking a look at it. If there are areas you're concerned about I'd like to fix them, so please let me know.
Assignee | ||
Comment 34•15 years ago
|
||
Chris let me know when you have the code ready (or if you need a hand). We'll be able to land without another review.
Comment 35•15 years ago
|
||
See also, bug 521215. both implementations appear relatively complete but unrelated code and authors... doh?
Comment 36•15 years ago
|
||
(In reply to comment #35)
> See also, bug 521215. both implementations appear relatively complete but
> unrelated code and authors... doh?
correction: see bug 531215
Comment 37•15 years ago
|
||
> > - As with all the other mode-selections, hard-wiring to LE/BE mode with CPP
> > symbols seems undesirable. Would prefer a runtime flag.
>
> Hmm... I thought that 64 bit values were stored in native format and the JIT
> needs to use the same ordering so I'm not clear why this decision should be
> made at runtime?
I tend to agree with Chris. runtime detection of optional cpu features or processor revisions is desireable. But, do we need a build of the VM compiled for LE and the JIT generating BE code (or vice versa)?
To follow convention, runtime code that tests the setting (with the setting declared as a static const) keeps the generator code ifdef-free while still making it a compile-time choice.
Reporter | ||
Comment 38•15 years ago
|
||
Updated MIPS backend for nanojit-central/tamarin-redux
Follows the NativeARM implementation where possible to ease maintenance
Attachment #410414 -
Attachment is obsolete: true
Reporter | ||
Comment 39•15 years ago
|
||
Reporter | ||
Comment 40•15 years ago
|
||
Assignee | ||
Comment 41•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-nanojit
Comment 42•15 years ago
|
||
Is there a plan for a MIPS machine in the TR buildbot setup? This back-end will break early and often if there isn't one.
Comment 43•15 years ago
|
||
Yes, bug 524129 addresses the build scripts, and we have the hardware in house here at Adobe.
Comment 44•15 years ago
|
||
Hi Chris, great work!
One thing is that regexp-dna will be rather slow without patch below. It is due to loading word from a 2-byte-aligned address, which causes slow exception processing. On Loongson machine, regexp-dna gets 9X faster with patch below.
Another way to fix this is probably to explicitly use unaligned load/store.
--- a/js/src/jsregexp.cpp Mon Jan 04 15:51:13 2010 +0800
+++ b/js/src/jsregexp.cpp Thu Jan 07 11:50:45 2010 +0800
@@ -2322,7 +2322,7 @@
return pos;
}
-#if defined(AVMPLUS_ARM) || defined(AVMPLUS_SPARC)
+#if defined(AVMPLUS_ARM) || defined(AVMPLUS_SPARC) || defined (AVMPLUS_MIPS)
/* We can't do this on ARM or SPARC, since it relies on doing a 32-bit load
from
* a pointer which is only 2-byte aligned.
*/
Comment 45•15 years ago
|
||
Perhaps we should add a generic flag to nanojit (NJ_UNALIGNED_ACCESS) so we don't have to sniff for specific processors.
Comment 46•15 years ago
|
||
Chris, I assume you'll be the goto person for any MIPS issues in the future? Eg. the interface between the backends and the rest of Nanojit changes reasonably often. When I'm the one causing such a change I try to fix all the back-ends appropriately, but for the case where I don't have machines (PPC, Sparc, MIPS) I sometimes cause breakage. We have a goto person at SUN for Sparc.
(I admit to not being thrilled at the addition of the MIPS back-end. To me it's just extra maintenance work with no upside. Who will actually be using this back-end?)
Comment 47•15 years ago
|
||
(In reply to comment #46)
> (I admit to not being thrilled at the addition of the MIPS back-end. To me
> it's just extra maintenance work with no upside. Who will actually be using
> this back-end?)
Supposedly MIPS is heavily used in some classes of consumer devices, like television sets.
Comment 48•15 years ago
|
||
(In reply to comment #47)
>
> Supposedly MIPS is heavily used in some classes of consumer devices, like
> television sets.
Is Nanojit running on these machines?
Comment 49•15 years ago
|
||
IIRC Bloomberg was doing a MIPS nanojit/TM port. Also Sparc, w00t.
Nick, don't be busting on MIPS, my favorite RISC architecture! *nostalgic sigh for SGI iron...* ;-)
/be
Comment 50•15 years ago
|
||
(In reply to comment #48)
> Is Nanojit running on these machines?
Obviously not, since (up to now) MIPS support wasn't present... :-)
Comment 51•15 years ago
|
||
(In reply to comment #48)
> (In reply to comment #47)
> >
> > Supposedly MIPS is heavily used in some classes of consumer devices, like
> > television sets.
>
> Is Nanojit running on these machines?
Nanojit is not running on MIPS yet to my knowledge, but variants of the Flash Platform are running on these types of devices: http://www.adobe.com/aboutadobe/pressroom/pressreleases/200904/042009FlashDigitalHome.html
Reporter | ||
Comment 52•15 years ago
|
||
(In reply to comment #46)
Yes. I am the tech contact at MIPS. We have an active interest in keeping the backend current.
Chris
Comment 53•15 years ago
|
||
BTW, we don't marked bugs as RESOLVED until they've been merged into Tamarin-Redux and mozilla-central.
http://hg.mozilla.org/tracemonkey/rev/fee1af2ffc8f
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
Comment 54•15 years ago
|
||
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
Comment 55•15 years ago
|
||
(In reply to comment #46)
>
> (I admit to not being thrilled at the addition of the MIPS back-end. To me
> it's just extra maintenance work with no upside. Who will actually be using
> this back-end?)
MIPS-based CPUs ship in > 500MU annually. Much of this is in DTV, STB, DVD/BD, and home networking gear. Belated, but thought this question should be answered ;<)
Comment 56•15 years ago
|
||
(In reply to comment #55)
>
> MIPS-based CPUs ship in > 500MU annually. Much of this is in DTV, STB, DVD/BD,
> and home networking gear. Belated, but thought this question should be answered
> ;<)
How many of those will be running Nanojit?
Comment 57•15 years ago
|
||
SoCs based on the MIPS architecture account for #1 market share in each of those segments. If you believe Flash player technology migrates into the living room, it's a large number of those shipments, dependent on the adoption rate of Flash and other apps using the JIT.
Comment 58•15 years ago
|
||
We don't have test infrastructure for mips, but if there are contributors willing to keep it working, I think we should take a patch. If we add LIR opcodes, it might break. We will need someone to watch out for breakage. Sun does this for SPARC. They run a tinderbox and if something break, Leon comes around and fixes it. On the upside, this is probably the last relevant architecture we have to add =)
Comment 59•15 years ago
|
||
Chris Dearman is the MIPS champion responsible for fixing broken builds. MIPS will be added to the build system once Chris addresses some recent breakage.
Comment 60•15 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•