Closed
Bug 985881
Opened 10 years ago
Closed 9 years ago
IonMonkey MIPS: Add support for N32 ABI to IonMonkey
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: rankov, Assigned: hev)
References
Details
Attachments
(1 file, 3 obsolete files)
6.03 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
This bug should track upstream process of IonMonkey MIPS N32 port. This port is made by Heiher. He has used my MIPS O32 port as a base. The code is available at: https://github.com/heiher/gecko-dev
Assignee | ||
Comment 1•10 years ago
|
||
(In reply to Branislav Rankov from comment #0) > This bug should track upstream process of IonMonkey MIPS N32 port. This port > is made by Heiher. He has used my MIPS O32 port as a base. > > The code is available at: https://github.com/heiher/gecko-dev Thanks for your help.
Assignee | ||
Comment 2•10 years ago
|
||
This patch added MIPS N32 ABI support, please review.
Attachment #8394075 -
Flags: review?(nfroyd)
Assignee | ||
Updated•10 years ago
|
Attachment #8394075 -
Flags: review?(nicolas.b.pierron)
Comment 3•10 years ago
|
||
Comment on attachment 8394075 [details] [diff] [review] Architecture-mips.patch Review of attachment 8394075 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/mips/Architecture-mips.h @@ +262,5 @@ > f28, > f30, > +#elif defined(USES_N32_ABI) > + f0 = 0, // f0, f2 - Return values > + f1, I have to say no, because having a #if in such a central point of the architecture sounds hard to follow, and thus penalize the review & quality. We are currently looking at handling correctly the float32 register and their aliasing. I think we should wait on these changes to mature to get this change in both O32_ABI and N32_ABI. Another hacky solution, would be to mark odd float registers as non allocatable and fake the 64 bits nature of even registers. In which case we can first enable all float for both O32_ABI and N32_ABI, and then specialize the call convention.
Attachment #8394075 -
Flags: review?(nicolas.b.pierron) → review-
Comment 4•10 years ago
|
||
Comment on attachment 8394075 [details] [diff] [review] Architecture-mips.patch Review of attachment 8394075 [details] [diff] [review]: ----------------------------------------------------------------- Nicolas has the final say, since he's the JS engine expert here. Giving it r+ because the ABI details look fine, but this can't go in until the details around the floating-point registers get straightened out. ::: js/src/jit/mips/Architecture-mips.cpp @@ +35,5 @@ > fclose(fp); > if (strstr(buf, "FPU")) > flags |= HWCAP_FPU; > + if (strstr(buf, "Loongson-3")) > + flags |= HWCAP_LOONGSON; What's the Loongson-specific bits coming in future patches, out of curiousity? ::: js/src/jit/mips/Architecture-mips.h @@ +21,5 @@ > +#if _MIPS_SIM == _ABIO32 > +#define USES_O32_ABI > +#elif _MIPS_SIM == _ABIN32 > +#define USES_N32_ABI > +#endif You probably want an: #else #error "unsupported ABI" here to prevent people from trying this on N64/O64/EABI/yet another MIPS ABI.
Attachment #8394075 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #3) > Comment on attachment 8394075 [details] [diff] [review] > Architecture-mips.patch > > Review of attachment 8394075 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit/mips/Architecture-mips.h > @@ +262,5 @@ > > f28, > > f30, > > +#elif defined(USES_N32_ABI) > > + f0 = 0, // f0, f2 - Return values > > + f1, > > I have to say no, because having a #if in such a central point of the > architecture sounds hard to follow, and thus penalize the review & quality. > > We are currently looking at handling correctly the float32 register and > their aliasing. I think we should wait on these changes to mature to get > this change in both O32_ABI and N32_ABI. > > Another hacky solution, would be to mark odd float registers as non > allocatable and fake the 64 bits nature of even registers. In which case we > can first enable all float for both O32_ABI and N32_ABI, and then specialize > the call convention. Thanks, I will modify my patches and submit them again after Branislav updated his port.
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Nathan Froyd (:froydnj) from comment #4) > Comment on attachment 8394075 [details] [diff] [review] > Architecture-mips.patch > > Review of attachment 8394075 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nicolas has the final say, since he's the JS engine expert here. Giving it > r+ because the ABI details look fine, but this can't go in until the details > around the floating-point registers get straightened out. > > ::: js/src/jit/mips/Architecture-mips.cpp > @@ +35,5 @@ > > fclose(fp); > > if (strstr(buf, "FPU")) > > flags |= HWCAP_FPU; > > + if (strstr(buf, "Loongson-3")) > > + flags |= HWCAP_LOONGSON; > > What's the Loongson-specific bits coming in future patches, out of > curiousity? Thanks. In my port, I'm using Loongson-specific instructions for optimization depends this flag. I will remove it and add a separate patch at next submit. > > ::: js/src/jit/mips/Architecture-mips.h > @@ +21,5 @@ > > +#if _MIPS_SIM == _ABIO32 > > +#define USES_O32_ABI > > +#elif _MIPS_SIM == _ABIN32 > > +#define USES_N32_ABI > > +#endif > > You probably want an: > > #else > #error "unsupported ABI" > > here to prevent people from trying this on N64/O64/EABI/yet another MIPS ABI. I will add it at next, thanks!
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8394075 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8406803 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8406804 -
Flags: review?(nicolas.b.pierron)
Attachment #8406804 -
Flags: review?(nfroyd)
Comment 9•10 years ago
|
||
I think we should first get the O32 MIPS backend settled and checked before making additional modification to it. As far as I understand, this modification implies that we have 64 bits registers, but only 32 bits pointers, right? If so this looks a lot like x32 ABI[1], and as far as I know Firefox (Iceweasel) does not even compile on such architectures[2]. [1] http://en.wikipedia.org/wiki/X32_ABI [2] https://lwn.net/Articles/548838/
Comment 10•10 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #9) > I think we should first get the O32 MIPS backend settled and checked before > making additional modification to it. > > As far as I understand, this modification implies that we have 64 bits > registers, but only 32 bits pointers, right? If so this looks a lot like > x32 ABI[1], and as far as I know Firefox (Iceweasel) does not even compile > on such architectures[2]. > > [1] http://en.wikipedia.org/wiki/X32_ABI > [2] https://lwn.net/Articles/548838/ Yes, N32 has 32-bit pointers and 64-bit registers. Firefox/Iceweasel likely doesn't build because nobody has made appropriate tweaks to x86-64-specific code to make it work for x32, or the appropriate modifications to build bits that think you can only have i386 and x86-64 on x86 chips. At least XPCOM appears to have some support for N32, so supporting x32 certainly isn't out of the question. The JIT might need to be taught about many x32-specific things, though...
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #9) > I think we should first get the O32 MIPS backend settled and checked before > making additional modification to it. > > As far as I understand, this modification implies that we have 64 bits > registers, but only 32 bits pointers, right? If so this looks a lot like > x32 ABI[1], and as far as I know Firefox (Iceweasel) does not even compile > on such architectures[2]. > > [1] http://en.wikipedia.org/wiki/X32_ABI > [2] https://lwn.net/Articles/548838/ Do you mean I should submit these patches after all O32 patches merged and stabled? You are right, It has 32-bit pointers and 64-bit registers. So, I think there have two style implement for N32: 1. Like O32, JS_NUNBOX32, use a paire of 32-bit GPRs (just low 32-bit) to boxing a JSValue. 2. Like N64, JS_PUNBOX64, use a GPR (64-bit) to boxing a JSValue. A good first, as you said. I selected JS_NUNBOX32 style for MIPS N32, because MIPS o32 already supported and JS_PUNBOX64 just for 64-bit in Value.h
Assignee | ||
Comment 12•10 years ago
|
||
Currently, Firefox 31.0a1 works for MIPS n32 ABI on Loongson3 platform. There is the code repo => https://github.com/heiher/gecko-dev and a screenshot of Octane 2.0 benchmark => http://bbs.lemote.com/attachments/14031608377d6b3565197f353e.png
Comment 13•10 years ago
|
||
(In reply to Heiher from comment #11) > (In reply to Nicolas B. Pierron [:nbp] from comment #9) > > I think we should first get the O32 MIPS backend settled and checked before > > making additional modification to it. > > Do you mean I should submit these patches after all O32 patches merged and > stabled? Yes, I think this is the best approach here, because we do not want to break O32 backend by mistake before being able to check for it with a simulator. > You are right, It has 32-bit pointers and 64-bit registers. So, I think > there have two style implement for N32: 1. Like O32, JS_NUNBOX32, use a > paire of 32-bit GPRs (just low 32-bit) to boxing a JSValue. 2. Like N64, > JS_PUNBOX64, use a GPR (64-bit) to boxing a JSValue. > > A good first, as you said. I selected JS_NUNBOX32 style for MIPS N32, > because MIPS o32 already supported and JS_PUNBOX64 just for 64-bit in Value.h This is exactly the kind of thing that I found scary. The NUNBOXing is not made to be compatible with the PUNBOXing. I do not think that having 2 boxing mechanism is ideal. And before looking at any patch in the JIT I would be interested in seeing patches in the rest of the engine, and ask Jason what he thinks about it.
Flags: needinfo?(jorendorff)
Comment 14•10 years ago
|
||
Comment on attachment 8406804 [details] [diff] [review] Architecture-mips.patch Review of attachment 8406804 [details] [diff] [review]: ----------------------------------------------------------------- Canceling this review for now; once we have all the O32 bits in, please r? again. I can't tell whether Nicolas will reject this patch based on the presence of many #ifs even after the first review. ;)
Attachment #8406804 -
Flags: review?(nfroyd)
Comment 15•10 years ago
|
||
Hello, Heiher. I can't tell how far along your work is. Does the JS interpreter work already? That is, can you build SpiderMonkey standalone and run tests? Instructions are at <https://developer.mozilla.org/en-US/docs/SpiderMonkey/Build_Documentation#Developer_build> and <https://developer.mozilla.org/en-US/docs/SpiderMonkey/Build_Documentation#Test>. You can configure with --disable-ion --disable-yarr-jit to start. If not, definitely get that working first before touching the JITs. I'm neutral on JS_PUNBOX64 vs JS_NUNBOX32. The interpreter really doesn't care. So that will be up to the JIT folks, like nbp; jandem has the last call as JIT module owner.
Flags: needinfo?(jorendorff)
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #15) > Hello, Heiher. > > I can't tell how far along your work is. > > Does the JS interpreter work already? That is, can you build SpiderMonkey > standalone and run tests? Instructions are at > <https://developer.mozilla.org/en-US/docs/SpiderMonkey/ > Build_Documentation#Developer_build> and > <https://developer.mozilla.org/en-US/docs/SpiderMonkey/ > Build_Documentation#Test>. You can configure with --disable-ion > --disable-yarr-jit to start. > > If not, definitely get that working first before touching the JITs. > > I'm neutral on JS_PUNBOX64 vs JS_NUNBOX32. The interpreter really doesn't > care. So that will be up to the JIT folks, like nbp; jandem has the last > call as JIT module owner. Hello, Currently, The SpiderMonkey standalone in MIPS N32 ABI (without any disable jit options) has been works on Loongson3 (MIPS64r2 complatible) platform. TESTS results: js/src/jit-tests: ALL PASSED js/src/tests: REGRESSIONS ecma_5/RegExp/regress-617935.js ecma_5/RegExp/regress-617935.js ecma_5/RegExp/regress-617935.js ecma_5/RegExp/regress-617935.js ecma_5/RegExp/regress-617935.js ecma_5/RegExp/regress-617935.js ecma_5/RegExp/regress-617935.js ecma_5/misc/new-with-non-constructor.js ecma_5/misc/new-with-non-constructor.js ecma_5/Function/builtin-no-construct.js ecma_5/Function/builtin-no-construct.js FAIL This is the code repo => https://github.com/heiher/gecko-dev Thanks!
Comment 17•10 years ago
|
||
Comment on attachment 8406804 [details] [diff] [review] Architecture-mips.patch Review of attachment 8406804 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/mips/Architecture-mips.h @@ +300,5 @@ > + // :TODO: (Bug 972836) // Fix this once odd regs can be used as float32 > + // only. For now we don't allocate odd regs for O32 ABI. > + 14; > +#elif defined(USES_N32_ABI) > + 30; No, this should be 13, as we are not able to allocate Float32 yet, and you are using 2 (should use only 1, see below) Float32 registers as additional scratch registers. @@ +323,5 @@ > + (1 << FloatRegisters::f25) | > + (1 << FloatRegisters::f27) | > + (1 << FloatRegisters::f29) | > + (1 << FloatRegisters::f31) | > +#endif As we are not allocating odd registers yet, this is a no-op. @@ +348,4 @@ > static const uint32_t NonAllocatableMask = > +#if defined(USES_O32_ABI) > + // :TODO: (Bug 972836) // Fix this once odd regs can be used as float32 > + // only. For now we don't allocate odd regs for O32 ABI. nit: no // inside the comment. @@ +371,5 @@ > +#elif defined(USES_N32_ABI) > + // f21 and f23 are MIPS scratch float registers. > + (1 << FloatRegisters::f21) | > + (1 << FloatRegisters::f23); > +#endif As we do not have real Float32 support yet, I suggest you are using f22 and f23 as Float32 scratch register. You at least need to remove the even counter part of the register to prevent it from being allocated by the register allocator.
Attachment #8406804 -
Flags: review?(nicolas.b.pierron)
Reporter | ||
Comment 18•10 years ago
|
||
(In reply to {N/A until Apr. 27} Nicolas B. Pierron [:nbp] from comment #17) > As we do not have real Float32 support yet, I suggest you are using f22 and > f23 as Float32 scratch register. > You at least need to remove the even counter part of the register to prevent > it from being allocated by the register allocator. We can use odd registers on N32 because on N32 all 32 registers can be used as both float32 and float64. This is because N32 is used only on MIPS64 CPU-s. There is no overlapping of odd and even registers on N32, so we can use all registers safely. The O32 uses float registers the way they are made on MIPS32 CPU-s. Also, O32 can be used on MIPS64 CPU-s because they have FLOAT32 mode for backward compatibility.
Comment 19•10 years ago
|
||
(In reply to Branislav Rankov from comment #18) > (In reply to {N/A until Apr. 27} Nicolas B. Pierron [:nbp] from comment #17) > > As we do not have real Float32 support yet, I suggest you are using f22 and > > f23 as Float32 scratch register. > > You at least need to remove the even counter part of the register to prevent > > it from being allocated by the register allocator. > > We can use odd registers on N32 because on N32 all 32 registers can be used > as both float32 and float64. This is because N32 is used only on MIPS64 > CPU-s. There is no overlapping of odd and even registers on N32, so we can > use all registers safely. > > The O32 uses float registers the way they are made on MIPS32 CPU-s. Also, > O32 can be used on MIPS64 CPU-s because they have FLOAT32 mode for backward > compatibility. Ok, then this need to appear as a comment in the code, especially with registers named f*.
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8409969 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Updated•10 years ago
|
Attachment #8406804 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8409969 -
Flags: review?(nicolas.b.pierron) → review+
Updated•9 years ago
|
Flags: needinfo?(r)
Comment 22•9 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #21) > Should this not have landed? Yes.
Assignee | ||
Comment 23•9 years ago
|
||
Thanks for your help, Nicolas, Jonathan! I want to implement jit support for MIPS n32 in MIPS64. It looks better than in MIPS 32. So I closed the bug.
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: needinfo?(r)
Resolution: --- → WONTFIX
Updated•9 years ago
|
Flags: needinfo?(nicolas.b.pierron)
You need to log in
before you can comment on or make changes to this bug.
Description
•