Closed Bug 985881 Opened 6 years ago Closed 5 years ago

IonMonkey MIPS: Add support for N32 ABI to IonMonkey

Categories

(Core :: JavaScript Engine: JIT, enhancement)

Other
Linux
enhancement
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: rankov, Assigned: hev)

References

Details

Attachments

(1 file, 3 obsolete files)

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: nobody → r
Depends on: 969375
(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.
Attached patch Architecture-mips.patch (obsolete) — Splinter Review
This patch added MIPS N32 ABI support, please review.
Attachment #8394075 - Flags: review?(nfroyd)
Attachment #8394075 - Flags: review?(nicolas.b.pierron)
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 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+
(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.
(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!
Attached patch Architecture-mips.patch (obsolete) — Splinter Review
Attachment #8394075 - Attachment is obsolete: true
Attached patch Architecture-mips.patch (obsolete) — Splinter Review
Attachment #8406803 - Attachment is obsolete: true
Attachment #8406804 - Flags: review?(nicolas.b.pierron)
Attachment #8406804 - Flags: review?(nfroyd)
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/
(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...
(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
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
(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 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)
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)
Depends on: 997274
(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 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)
(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.
(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*.
Attachment #8409969 - Flags: review?(nicolas.b.pierron)
Attachment #8406804 - Attachment is obsolete: true
Attachment #8409969 - Flags: review?(nicolas.b.pierron) → review+
Should this not have landed?
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(r)
(In reply to Jonathan Watt [:jwatt] from comment #21)
> Should this not have landed?

Yes.
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: 5 years ago
Flags: needinfo?(r)
Resolution: --- → WONTFIX
Flags: needinfo?(nicolas.b.pierron)
You need to log in before you can comment on or make changes to this bug.