458060 - port nanojit to sparc

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Comment 1

[Steven Johnson]

There is some refactoring already underway for the PowerPC port (https://bugzilla.mozilla.org/show_bug.cgi?id=458077) that will probably make this jump much simpler.

Comment 2

[Leon Sha]

Attachment: patch

I made this patch based on mozilla-central. I use xpcshell to test the trace-test.js. Most of them are passed. Since currently there are some compiler problem on tamarin-central, I didn't try this on tamarin-central yet. Also I don't know if I need to test it on tracemonkey branch. Acturally I am a little confused with tamarin-central, tamarin-tracing, mozilla-central and tracemonkey tree.

Comment 3

[Edwin Smith]

tamarin-tracing is not in active development.

here's the relationships for the other branches:

                stable               development
                ------               -----------
tamarin         tamarin-central      tamarin-redux
spidermonkey    mozilla-central      tracemonkey

A patch based on tamarin-redux would be ideal.  In particular, Assembler.cpp in tamarin-redux was recently cleaned up to move [almost] all cpu-specific code into cpu specific files (Native___.[cpp,h])

Comment 4

[Rick Reitmaier]

Comment on attachment 343205 [details] [diff] [review]
patch

awaiting leon's rebased patch

Comment 5

[Leon Sha]

Attachment: Patch based on tamarin-redux tree.

This patch is based on tamarin-redux tree. Should I also work out a patch for tracemonkey tree or it will merge to tracemonkey automatically?
In this patch I modified some code other than Nativesparc.*.
1. Assembler.cpp
I removed the comments in if since it make my compiler failed also it don't look important.
2. LIR.h
Something should inside BIG_ENDIAN. I use NANOJIT_SPARC instead. AVMPLUS_BIG_ENDIAN can't be used because AVMPLUS_BIG_ENDIAN is not defined inside nanojit.
3. Native.h
I made some change in asm_output.

Comment 6

[Steven Johnson]

We really need to remove avmplus dependencies from nanojit.

If NJ doesn't have it's own BIG_ENDIAN define, it needs one -- PowerPC will also need it.

Comment 7

[Rick Reitmaier]

Comment on attachment 345467 [details] [diff] [review]
Patch based on tamarin-redux tree.

This looks good and you should be targeting tamarin-redux.  Tracemonkey is pulling from redux so when we land this patch, they will be able to pick it up also.  

endian defines were added recently and you can take advantage of them once you rebase your code to the latest redux.

Also, the output macros in native.h shouldn't be touched, if possible.

Marking '-' for now since code requires a couple of fix-ups.

Comment 8

[Rick Reitmaier]

Also forgot to mention, that we should make sure we have a full pass of the test suite located in test/acceptance, if you haven't already done so.   

All the tests are now running successfully under all platforms, so there shouldn't be any platform independent issues with the suite.

Comment 9

[Leon Sha]

Th(In reply to comment #7)
> (From update of attachment 345467 [details] [diff] [review])
> This looks good and you should be targeting tamarin-redux.  Tracemonkey is
> pulling from redux so when we land this patch, they will be able to pick it up
> also.  
> 
Yes, this patch is based on tamarin-redux.

> endian defines were added recently and you can take advantage of them once you
> rebase your code to the latest redux.
> 
I didn't find it in nanojit. Maybe it is not in the tree yet. I'll redo the patch after that.

> Also, the output macros in native.h shouldn't be touched, if possible.
> 
> Marking '-' for now since code requires a couple of fix-ups.
The reason that I change asm_output is it is defined as asm_output(f, ...). This doesn't work for our complier if you just use asm_output("something"). I need change all these to asm_output("%s", "something"). At least there should be a comma inside asm_output. I don't think the FMT inside asm_output is really needed.

Also I have passed all the test acceptance after the patch. I'll also make sure the test/acceptance be passed next time.

Comment 10

[Steven Johnson]

> The reason that I change asm_output is it is defined as asm_output(f, ...).
> This doesn't work for our complier if you just use asm_output("something"). I
> need change all these to asm_output("%s", "something"). At least there should
> be a comma inside asm_output. I don't think the FMT inside asm_output is really
> needed.

Does the SunPro compiler not support variadic macros? I thought pretty much every compiler did these days (even MSVC and CodeWarrior)... 

http://en.wikipedia.org/wiki/Variadic_macro

Comment 11

[Leon Sha]

(In reply to comment #10)
> > The reason that I change asm_output is it is defined as asm_output(f, ...).
> > This doesn't work for our complier if you just use asm_output("something"). I
> > need change all these to asm_output("%s", "something"). At least there should
> > be a comma inside asm_output. I don't think the FMT inside asm_output is really
> > needed.
> 
> Does the SunPro compiler not support variadic macros? I thought pretty much
> every compiler did these days (even MSVC and CodeWarrior)... 
> 
> http://en.wikipedia.org/wiki/Variadic_macro
It support variadic macros. But if you define like asm_output(f, ...), there should be at least two params, since there is a 'f' defined inside asm_output. If you just defined asm_output(...), there will be no problem.

Comment 12

[Rick Reitmaier]

- I validated the ... change on osx and it works; I need to check the other platforms but let's assume for now that its ok.

- AVMPLUS_BIG_ENDIAN is defined in the code so you should be able to use that for the Lir.h changes.

- not related to this patch but would mind to check out these solaris specific items to see if they are still needed;
   avmbuild.h has a ifdef SOLARIS; is it needed.
   nativei386 #ifdefined SOLARIS

- also the changes in emitImt thunk need to be abstracted out (somehow) but for this patch we can leave them as is.  I'm not yet sure how best to do this, but we'll look at it post-patch.

Comment 13

[Leon Sha]

(In reply to comment #12)
> - I validated the ... change on osx and it works; I need to check the other
> platforms but let's assume for now that its ok.
> 
> - AVMPLUS_BIG_ENDIAN is defined in the code so you should be able to use that
> for the Lir.h changes.
> 
AVMPLUS_BIG_ENDIAN is defined in avmbuild.h which is not included by tracemonkey and maybe other project who just want to use nanojit. Is that OK we define a NJ_BIG_ENDIAN inside nanojit.h?

> - not related to this patch but would mind to check out these solaris specific
> items to see if they are still needed;
>    avmbuild.h has a ifdef SOLARIS; is it needed.
>    nativei386 #ifdefined SOLARIS
Yes they are still needed.
> 
> - also the changes in emitImt thunk need to be abstracted out (somehow) but for
> this patch we can leave them as is.  I'm not yet sure how best to do this, but
> we'll look at it post-patch.

Comment 14

[Steven Johnson]

I agree, we shouldn't have nanojit depend on avmbuild.h (or any other avm-specific header). seems simple enough to accomplish and a useful separation.

Comment 15

[Leon Sha]

Attachment: patch v3

I defined NANOJIT_BIG_ENDIAN and NANOJIT_LITTLE_ENDIAN in nanojit.h.

Comment 16

[Rick Reitmaier]

While I don't believe its critical to landing the patch, I was trying to think of a way that we could abstract the CodegenLIR changes out.  Thoughts anyone?

Comment 17

[Steven Johnson]

Agreed, fine to land without it, but definitely should follow up to fix ASAP... having CodegenLIR be platform-ifdef-free is IMHO a goal we should take a hard line on, lest we end up like CodegenMIR again.

Also, once this has landed, let's add checks into avmplus to verify that the nanojit endian macros match the avmplus endian macros.

Comment 18

[Rick Reitmaier]

Leon, is there a set of patches we need to apply in order to get this to compile on sparc?

Brent tried the following sequence and couldn't build it:

Base: 1056 (chose 1056 because this is when TypeDescriber was added, fails to compile when using tip + solaris patches)
Patches applied:
https://bugzilla.mozilla.org/show_bug.cgi?id=463182 (already in source, but pushed AFTER 1056)
https://bugzilla.mozilla.org/show_bug.cgi?id=451150 (already in source, but pushed AFTER 1056)
https://bugzilla.mozilla.org/show_bug.cgi?id=458060 (SPARC nanojit patch)
https://bugzilla.mozilla.org/show_bug.cgi?id=463191 (SunStudio compiler hack around)

Maybe we should focus on getting sparc to build cleanly first, before applying this patch.  Can you give us an update on where sparc is at the moment or point to another bug that is tracking it?

Comment 19

[Leon Sha]

(In reply to comment #18)
> Leon, is there a set of patches we need to apply in order to get this to
> compile on sparc?
> 
> Brent tried the following sequence and couldn't build it:
> 
> Base: 1056 (chose 1056 because this is when TypeDescriber was added, fails to
> compile when using tip + solaris patches)
> Patches applied:
> https://bugzilla.mozilla.org/show_bug.cgi?id=463182 (already in source, but
> pushed AFTER 1056)
> https://bugzilla.mozilla.org/show_bug.cgi?id=451150 (already in source, but
> pushed AFTER 1056)
> https://bugzilla.mozilla.org/show_bug.cgi?id=458060 (SPARC nanojit patch)
> https://bugzilla.mozilla.org/show_bug.cgi?id=463191 (SunStudio compiler hack
> around)
> 
> Maybe we should focus on getting sparc to build cleanly first, before applying
> this patch.  Can you give us an update on where sparc is at the moment or point
> to another bug that is tracking it?

We need the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=463191 and the last attachment in https://bugzilla.mozilla.org/show_bug.cgi?id=451150 which is another hack. Also you need latest Sun Studio to build. Currently the tip builds OK with these patches.

Comment 20

[Brent Baker]

What is the latest SunStudio compiler? I am using the last released SunStudio 12 package (SunStudio12ml-solaris-sparc-200709-pkg.tar.bz2), are you using this or are you using the SunStudio Express from July 2008?

Comment 21

[Brent Baker]

-bash-3.00$ /usr/bin/version
Machine hardware:   sun4u
OS version:         5.10
Processor type:     sparc
Hardware:           SUNW,UltraAX-i2

The following components are installed on your system:


Sun Studio 12
        Sun Studio 12 C Compiler
        Sun Studio 12 C++ Compiler
        Sun Studio 12 Tools.h++ 7.1
        Sun Studio 12 C++ Standard 64-bit Class Library
        Sun Studio 12 Garbage Collector
        Sun Studio 12 Debugging Tools (including dbx)
        Sun Studio 12 Performance Analyzer (including collect, ...)
        Sun Studio 12 LockLint
        Sun Studio 12 Building Software (including dmake)
        Sun Studio 12 Documentation Set
        Sun Studio 12 /usr symbolic links and GNOME menu item

version of "/opt/SUNWspro/bin/../prod/bin/../../bin/cc": Sun C 5.9 SunOS_sparc Patch 124867-01 2007/07/12
version of "/opt/SUNWspro/bin/../prod/bin/../../bin/CC": Sun C++ 5.9 SunOS_sparc Patch 124863-01 2007/07/25
version of "/opt/SUNWspro/bin/../prod/bin/../../bin/dbx": Sun Dbx Debugger 7.6 SunOS_sparc Patch 124872-01 2007/07/12
version of "/opt/SUNWspro/bin/../prod/bin/../../bin/analyzer": Sun Analyzer 7.6 SunOS_sparc Patch 126995-01 2007/07/17
version of "/opt/SUNWspro/bin/../prod/bin/../../bin/dmake": Sun Distributed Make 7.8 SunOS_sparc Patch 126503-01 2007/07/19

Comment 22

[Leon Sha]

I mean sunstudio 12. This should work. You only need the hack patch from https://bugzilla.mozilla.org/show_bug.cgi?id=463191 before apply this patch. Everything should OK. If it doesn't work can you paste the error message here?

Comment 23

[Brent Baker]

Both of these configurations fail with the same assertion for me (although different line): 
1) tamarin-redux tip (1085) + patch from #463191 
ERROR:
 >> Assertion:   (../lnk/tmplmatchargs.cc, line 193)
    while processing core/TypeDescriber.i at line 15832.
make: *** [core/TypeDescriber.o] Error 1

2) tamarin-redux tip (1085) + patch from #463191 + patch_v3 from #458060 
ERROR:
 >> Assertion:   (../lnk/tmplmatchargs.cc, line 193)
    while processing core/TypeDescriber.i at line 15774.
make: *** [core/TypeDescriber.o] Error 1


$ ../configure.py --enable-shell
$ make clean
$ make

Compiles for awhile with lots of warnings about hiding object properties and then finally fails with this error:

CC -E -DAVMPLUS_CDECL -DAVMPLUS_SPARC -DMMGC_INTERIOR_PTRS -DUNIX -DSOLARIS -DSO
FT_ASSERTS -DAVMPLUS_UNIX    -xO5    -I/export/home/build/buildbot/tamarin-redux
/solaris_sparc/tamarin-redux -I/export/home/build/buildbot/tamarin-redux/solaris
_sparc/tamarin-redux/MMgc -I/export/home/build/buildbot/tamarin-redux/solaris_sp
arc/tamarin-redux/core -I/export/home/build/buildbot/tamarin-redux/solaris_sparc
/tamarin-redux/codegen -I/export/home/build/buildbot/tamarin-redux/solaris_sparc
/tamarin-redux/pcre  /export/home/build/buildbot/tamarin-redux/solaris_sparc/tam
arin-redux/core/TypeDescriber.cpp > core/TypeDescriber.i
/export/home/build/buildbot/tamarin-redux/solaris_sparc/tamarin-redux/build/depe
ndparser.py core/TypeDescriber.deps < core/TypeDescriber.i > /dev/null
CC -o core/TypeDescriber.o -DAVMPLUS_CDECL -DAVMPLUS_SPARC -DMMGC_INTERIOR_PTRS
-DUNIX -DSOLARIS -DSOFT_ASSERTS -DAVMPLUS_UNIX    -xO5    -I/export/home/build/b
uildbot/tamarin-redux/solaris_sparc/tamarin-redux -I/export/home/build/buildbot/
tamarin-redux/solaris_sparc/tamarin-redux/MMgc -I/export/home/build/buildbot/tam
arin-redux/solaris_sparc/tamarin-redux/core -I/export/home/build/buildbot/tamari
n-redux/solaris_sparc/tamarin-redux/codegen -I/export/home/build/buildbot/tamari
n-redux/solaris_sparc/tamarin-redux/pcre  -c core/TypeDescriber.i
"/export/home/build/buildbot/tamarin-redux/solaris_sparc/tamarin-redux/core/BitS
et.h", line 97: Warning: bits hides avmplus::BitSet::bits.
"/export/home/build/buildbot/tamarin-redux/solaris_sparc/tamarin-redux/core/BitS
et.h", line 136: Warning: bits hides avmplus::BitSet::bits.
"/export/home/build/buildbot/tamarin-redux/solaris_sparc/tamarin-redux/core/BitS
et.h", line 157: Warning: bits hides avmplus::BitSet::bits.
"/export/home/build/buildbot/tamarin-redux/solaris_sparc/tamarin-redux/core/Trai
ts.h", line 336: Warning: residingPool hides avmplus::TraitsMetadata::residingPo
ol.
"/export/home/build/buildbot/tamarin-redux/solaris_sparc/tamarin-redux/core/Trai
ts.h", line 527: Warning: name hides avmplus::Traits::name.
"/export/home/build/buildbot/tamarin-redux/solaris_sparc/tamarin-redux/core/Trai
ts.h", line 527: Warning: ns hides avmplus::Traits::ns.
"/export/home/build/buildbot/tamarin-redux/solaris_sparc/tamarin-redux/core/Trai
ts.h", line 528: Warning: name hides avmplus::Traits::name.
"/export/home/build/buildbot/tamarin-redux/solaris_sparc/tamarin-redux/core/Trai
ts.h", line 528: Warning: ns hides avmplus::Traits::ns.
"/export/home/build/buildbot/tamarin-redux/solaris_sparc/tamarin-redux/core/Date
Object.h", line 76: Warning: date hides avmplus::DateObject::date.
 >> Assertion:   (../lnk/tmplmatchargs.cc, line 193)
    while processing core/TypeDescriber.i at line 15774.
make: *** [core/TypeDescriber.o] Error 1

Comment 24

[Leon Sha]

This is the bug https://bugzilla.mozilla.org/show_bug.cgi?id=463191. Maybe the hack patch need to be updated. I'll check it and post another hack patch later.

Comment 25

[Leon Sha]

Brent: I have updated the patch in bug 463191. Can you try again? Thanks a lot.

Comment 26

[Brent Baker]

I was able to compile the release version using tip (1091) and lastest patch from bug 463191. 

Leon have you passed a full acceptance run with your changes? On the solaris machine that I have I had the following failures in the release version of the shell:

Following section of tests failied with "Illegal Instruction - core dumped":
as3/Definitions/Classes/ClassDef/PrivateStaticClassMethodAndProp.abc
as3/Definitions/Classes/ClassDef/StaticClassMethodAndProp.abc
as3/Definitions/Classes/Ext/DynExtDefaultClassPub.abc
as3/Definitions/Classes/Ext/DynExtPublicClassFin.abc
as3/Definitions/Classes/Ext/ExtInternalClass.abc
as3/Definitions/Classes/Ext/FinExtDefaultClassFin.abc
as3/Definitions/Classes/Ext/FinExtDefaultClassStat.abc
as3/Definitions/Classes/Ext/FinExtDynamicClassPub.abc
as3/Definitions/Classes/Ext/IntExtInternalClass.abc
as3/Definitions/Classes/Ext/PubFinExtDefaultClass.abc
as3/Definitions/Classes/ExtImpl/ExtDynamicImplDefDefStat.abc
as3/Definitions/Classes/Override/SuperDifferentNamespace.abc
as3/Definitions/Classes/Override/SuperSameNamespace.abc
as3/Definitions/Function/StaticFunctionBody.abc
as3/Definitions/Super/SuperExprChainAccess.abc
ecma3/Date/e15_9_5_34_1.abc
ecma3/ExecutionContexts/e10_1_4_10.abc
ecma3/Expressions/e11_8_1.abc
ecma3/Expressions/e11_8_2.abc
ecma3/FunctionObjects/e15_3_3_1_4.abc
ecma3/RegExp/e15_10_4_1_5_rt.abc
ecma3/String/e15_5_4_4_2.abc


Date failures: Date and timezone on machine were correct
ecma3/Date/e15_9_5_10_6.abc : (new Date(959832000000)).getDate() = false FAILED! expected: true
ecma3/Date/e15_9_5_28_1.abc : TDATE = new Date(946684800000);(TDATE).setMinutes(1234567);TDATE.getTime() = 1020758820000 FAILED! expected: 1020755220000
ecma3/Date/e15_9_5_28_1.abc : TDATE = new Date(946684800000);(TDATE).setMinutes(1234567);TDATE.valueOf() = 1020758820000 FAILED! expected: 1020755220000
ecma3/Date/e15_9_5_28_1.abc : TDATE = new Date(946684800000);(TDATE).setMinutes(1234567);TDATE.getUTCHours() = 8 FAILED! expected: 7
ecma3/Date/e15_9_5_29_1.abc : TDATE = new Date(946684800000);(TDATE).setUTCMinutes(1234567);TDATE.getHours() = 3 FAILED! expected: 4


VerifyError:
ecma3/Statements/eregress_74474_003.abc VerifyError: Error #1030

Comment 27

[Leon Sha]

I have passed the acceptance when I submit the patch. I'll try to fix them and update the patch.

Comment 28

[Leon Sha]

Attachment: test result

I didn't get the error you got. I don't know what's wrong. Maybe the the asc.jar is different? Or maybe it is because of the cpu differnece. I have written some code which is not work on your cpu? Is this machine inside Adobe? Maybe I can login into this machine and have a look since I have vpn access to adobe network.

Comment 29

[Brent Baker]

All testcase files were compiled using the latest (at the time) asc.jar (#4082)

ftp://ftp.mozilla.org/pub/js/tamarin/builds/asc/latest/asc.jar

If you still do not see the same issues after recompiling the abc files ping me offline and I will work with you to get access to the machine. (yes it is inside Adobe)

$ ./runtests.py --rebuildtests
or 
$ ./runtests.py -f

Comment 30

[Leon Sha]

Attachment: patch v4

I added clear instruction cache method in ::genPrologue. I copied this from Assembler.cpp. I think this method should inside every platform files not Assembler.cpp. There are still 5 test cases failed. They are related to date. Maybe the timezone in the test machines is different from the expected. Also there are a lot of assert in debug build. They are related to MMgc. I'd like to file another bug to issue this.

Comment 31

[Rick Reitmaier]

The timezone issues are known and there is a bug already on them, so I wouldn't worry about those passing.

The asserts in MMgc are somewhat worrying, they shouldn't be firing.  Can you provide a little more detail.

Comment 32

[Leon Sha]

You can view the log here.
http://tamarin-builds.mozilla.org/sandbox/builders/solaris-sparc/builds/7/steps/Testsuite_DebugDebugger/logs/stdio
There are three kind of assert here. I am still looking at this. But it is not easy.
   Assertion failed: "((ref->get() == 0))" ("/export/home/build/buildbot/sandbox/solaris-sparc/sandbox/MMgc/GCTests.cpp":95)
   Assertion failed: "((ref->get() == 0))" ("/export/home/build/buildbot/sandbox/solaris-sparc/sandbox/MMgc/GCTests.cpp":105)
   Assertion failed: "((false))" ("/export/home/build/buildbot/sandbox/solaris-sparc/sandbox/MMgc/GC.cpp":3315)

Comment 33

[Rick Reitmaier]

Not sure if you've been able to make progress on the above asserts yet or not; but FWIW the last one will fire when it appears to the gc that a pointer has escaped detection.  

This should fire when a write-barrier was not placed on a pointer, but it is prone to false-positives, meaning that it might fire inappropriately. 

The first two are possibly related to CleanStack() not actually clearing the stack properly.  Have a look at the semantics in that method and make sure the sparc build is doing the correct thing (does _getsp() actually provide the correct value?).

Comment 34

[Leon Sha]

I update the tree to the latest version, the third assert has gone. The first two asserts are related to CleanStack(). On sparc using alloca and memset does not really clean the stack. After I did a msync, it really clean the stack. With this change there is no assert any more. You can see the log here.
http://tamarin-builds.mozilla.org/sandbox/builders/solaris-sparc/builds/9
However there is still one unexpected failure here, I'll try to figure it out.

Comment 35

[Leon Sha]

Attachment: patch v5

I have checked the failure for as3/ShellClasses/toplevel.abc. System.privateMemory is not enabled for solaris yet.

Comment 36

[Rick Reitmaier]

The patch looks good, but we're blocked by 469804.  Any update on it?

Comment 37

[Brent Baker]

Wondering if there has been any updated news on when a new SunStudio compiler will be available (bug 469804).

Since a patch has been posted that enables g++ compilation on solaris (#476200), I think that we should move forward with compiling with g++ for now. This patch really needs to land in tamarin-redux before it gets too old. As it stands now it needs to be rebased as there have been changes to the nanojit that cause this patch to not apply cleanly.

Comment 38

[Wesley W. Garland]

Brent: How close is tamarin-redux to tracemonkey?  I've already modified this patch for use in tracemonkey as bug 476042. 

If taramin-redux has had the same changes in the nanojit directory as tracemonkey, applying my patch from 476042 to the nanojit directory would probably save you some typing.

Comment 39

[Leon Sha]

Sun patch 124863-09 fixed that problem. You can get this patch from sunsolve.sun.com.
I'll mark the bug 469804 as resolved.
(In reply to comment #37)
> Wondering if there has been any updated news on when a new SunStudio compiler
> will be available (bug 469804).
> 
> Since a patch has been posted that enables g++ compilation on solaris
> (#476200), I think that we should move forward with compiling with g++ for now.
> This patch really needs to land in tamarin-redux before it gets too old. As it
> stands now it needs to be rebased as there have been changes to the nanojit
> that cause this patch to not apply cleanly.

Comment 40

[Leon Sha]

For the current tamarin-redux tree, with Sun Studio patch 124863-09, you can build a usable avmshell. And it -interp is working already.
There are several problems remains here.
1. CleanStack() problem.
The method I used in this patch (msync) is not correct. We have a assembler version solution for this problem. But we don't know how to integrate it into current build system. I'll file another bug to address it.
2. CODEGEN_MIR problem.
Codegen_MIR is currently broken on tamarin-redux. I am trying to fix them. But if the Codegen_MIR is going to be obsoleted, I am wondering if I still need to take time to get it fixed.

I'll post the nanojit only patch based on the latest tree later.

Comment 41

[Edwin Smith]

It is not necessary to fix CodegenMIR unless you want to have it working to validate the output of CodegenLIR.  Once we have all the nanojit backends working we plan to delete codegen/CodegenMIR.* and codegen/*Assembler.* completely.  nanojit ftw!

Comment 42

[Leon Sha]

Attachment: patch

Redo the patch based on the latest tree.

Comment 43

[Rick Reitmaier]

Comment on attachment 360651 [details] [diff] [review]
patch

Marking (r-) because of a couple of minor changes needed, but rest looks good.

a) avmbuild.h - added defined(__i386); was this necessary ? What compiler did it show up on.

b) LIR.cpp - can we use AVMPLUS_xxx_ENDIAN and not introduce NANOJIT_xxx macros?  
See avmbuild.h for code that appears to do the right thing for sparc already.
(Note this also means that the non-sparc changes to nanojit.h are not needed).

c) Not critical but can we use the name NativeSparc or NativeSPARC,  to be consistent with the other file names.

Comment 44

[Edwin Smith]

I beleive a version of this patch has already been ported to tracemonkey and landed, so good idea to stick to whatever filename was used there, to minimize churn.

Comment 45

[Leon Sha]

__i386 is for Sun Studio Compiler. It only defined __i386 rather than __i386__. Anyway it is not a Sparc related issue, I'll remove this. The reason to introduce NANOJIT_xxx is that some projects like tracemonkey, they only use nanojit. So AVMPLUS_xxx_ENDIAN will not defined there. If we use AVMPLUS_xxx here, we need to patch all the projects which only use nanojit. For the file name, I prefer NativeSparc.*, I'll change it.

There is a version of patch for tracemonkey, but it is not landed yet. Bug 476042 is about this.

Comment 46

[Lars T Hansen]

We're trending towards using VMCFG_xxx for the internal names to control almost everything (reserving AVMPLUS_xxx for external configuration) but that fix has not landed yet.  So whatever you do there's likely some patching to be done for nanojit clients in the future.

Comment 47

[Mike Shaver (:shaver emeritus)]

TraceMonkey already sets AVMPLUS_IA32 and AVMPLUS_UNIX on Mac-x86, so adding some SPARC bits in there won't be fatal.  I wouldn't worry about it too much.

Comment 48

[Rick Reitmaier]

re: __i386 no problem, please leave it in then.

Comment 49

[Leon Sha]

Attachment: patch v 7

Patch based on the latest tree with Rick's suggestion.

Comment 50

[Rick Reitmaier]

Comment on attachment 361440 [details] [diff] [review]
patch v 7

excellent, let's get this in so MIR can be retired.

Comment 51

[Leon Sha]

http://hg.mozilla.org/tamarin-redux/rev/1f84b2caefb5