817943 - Improve register allocation in Ionmonkey's specialized DOM stuff

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See issue #3 in bug 817937.

I hacked this up on x86-64 using IntArgReg0 and company, but those are x86-64 specific.  I'm told ARM also has argument registers; could we define these there too?  What about others?  Seems like we could just randomly pick some of the CallTemp* regs on x86 too, right?

Comment 1

[Boris Zbarsky [:bzbarsky]]

And in particular, I can obviously make this work great on x86-64 by carefully picking my CallTemp* args there. But I'd rather it worked great on ARM at the same time.  ;)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: What I have so far, not compiling except on x86-64

Comment 3

[Marty Rosenberg [:mjrosenb]]

If we can make register specifications after the constructor's initializer list is done running, we can use the function static inline bool GetIntArgReg(uint32 intArg, uint32 floatArg, Register *out) which is currently defined on x64 (and a similar function is defined on ARM).  This will let us do a runtime test to see if  our architecture supports calling via registers.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 688102 [details] [diff] [review]
What I have so far, not compiling except on x86-64

Review of attachment 688102 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/assembler/assembler/X86Assembler.h
@@ +1354,5 @@
>  
>      void movl_rr(RegisterID src, RegisterID dst)
>      {
> +        if (src == dst) {
> +            // Nothing to do.  Just bail

The problem comes from the MoveResolver which should not produce such instructions.
Having this kind of instruction is even more error prone as we might depend on the size of instructions.

::: js/src/ion/Lowering.cpp
@@ +269,5 @@
>  
>      // Call DOM functions.
>      if (call->isDOMFunction()) {
>          JS_ASSERT(target && target->isNative());
> +        LCallDOMNative *lir = new LCallDOMNative(argslot, tempFixed(IntArgReg0),

I am not in favor of having IntArgReg used on all architecture.  I would prefer a common interface similar to GetIntArgReg which exists on all platform, except that it could return the last of the non-overlapping CallTemp if there  not enough Argument registers.

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #4)
> Comment on attachment 688102 [details] [diff] [review]
> What I have so far, not compiling except on x86-64
> 
> Review of attachment 688102 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/assembler/assembler/X86Assembler.h
> @@ +1354,5 @@
> >  
> >      void movl_rr(RegisterID src, RegisterID dst)
> >      {
> > +        if (src == dst) {
> > +            // Nothing to do.  Just bail
> 
> The problem comes from the MoveResolver which should not produce such
> instructions.
> Having this kind of instruction is even more error prone as we might depend
> on the size of instructions.
> 
> ::: js/src/ion/Lowering.cpp
> @@ +269,5 @@
> >  
> >      // Call DOM functions.
> >      if (call->isDOMFunction()) {
> >          JS_ASSERT(target && target->isNative());
> > +        LCallDOMNative *lir = new LCallDOMNative(argslot, tempFixed(IntArgReg0),
> 
> I am not in favor of having IntArgReg used on all architecture.  I would
> prefer a common interface similar to GetIntArgReg which exists on all
> platform, except that it could return the last of the non-overlapping
> CallTemp if there  not enough Argument registers.

I don't know why this function is named differently on ARM but this is GetArgReg in ion/arm/Assembler-arm.h

ARM registers are use as follow:
4 x 32b: r0, r1, r2, r3
64b+32b: r0, r1, r2
32b+64b: r0,     r2, r3

If I remember correctly, but don't worry, we almost don't have 64b values as arguments anymore, except for double.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> The problem comes from the MoveResolver which should not produce such instructions.

I considered that; mjrosenberg suggested that fixing it at a lower level would catch more cases.  Apparently that's what the ARM code does (the check there is in ma_mov).

But in any case, the patch attached is not a serious proposal, just something I was experimenting with.  I agree that we need a better setup to work well with i386 for one thing...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Introduce a GetTempRegForIntArg function that can be used to get registers to store things you plan to pass to a function call in.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Use GetTempRegForIntArg for DOM getters/setters/methods.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Nicolas, let me know what you want me to do with the bit that prevents same-register moves on x86-64?  Separate bug, or trying to nix them in the MoveResolver or what?

Comment 10

[Nicolas B. Pierron [:nbp]]

Comment on attachment 695109 [details] [diff] [review]
part 1.  Introduce a GetTempRegForIntArg function that can be used to get registers to store things you plan to pass to a function call in.

Review of attachment 695109 [details] [diff] [review]:
-----------------------------------------------------------------

Fold this patch with the next one, to avoid patch adding dead code.

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 695110 [details] [diff] [review]
part 2.  Use GetTempRegForIntArg for DOM getters/setters/methods.

Review of attachment 695110 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds good.

::: js/src/ion/Lowering.cpp
@@ +295,5 @@
> +        MOZ_ASSERT(ok, "How can we not have three temp registers?");
> +        ok = GetTempRegForIntArg(3, 0, &argcReg);
> +        MOZ_ASSERT(ok, "How can we not have four temp registers?");
> +        ok = GetTempRegForIntArg(4, 0, &valueReg);
> +        MOZ_ASSERT(ok, "How can we not have five temp registers?");

nit: Remove all MOZ_ASSERT except the last one because if GetTempRegForIntArg(N, 0, …) fails, then GetTempRegForIntArg(N + 1, 0, …) fails too.

@@ +2131,5 @@
> +    Register tempReg1, tempReg2;
> +    ok = GetTempRegForIntArg(2, 0, &tempReg1);
> +    MOZ_ASSERT(ok, "How can we not have five temp registers?");
> +    ok = GetTempRegForIntArg(3, 0, &tempReg2);
> +    MOZ_ASSERT(ok, "How can we not have six temp registers?");    

nit: remove trailing whitespace.

Comment 12

[Nicolas B. Pierron [:nbp]]

(In reply to Boris Zbarsky (:bz) from comment #9)
> Nicolas, let me know what you want me to do with the bit that prevents
> same-register moves on x86-64?  Separate bug, or trying to nix them in the
> MoveResolver or what?

In the current state, I think it would be easy to add this to the passWithABI logic.  Hopefully we should be able to assert that we don't add such no-op move in the MoveResolver to ensure that our register allocators are not producing this kind of weird moves.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for the register moves part, per comment 12

This passes tests on try, so the assert isn't triggering...

Comment 14

[Nicolas B. Pierron [:nbp]]

Comment on attachment 695554 [details] [diff] [review]
Patch for the register moves part, per comment 12

Review of attachment 695554 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/MoveResolver.cpp
@@ +27,5 @@
> +    // Assert that we're not doing no-op register moves.  Note that we don't
> +    // want to assert about same-address memory moves, because those may be hard
> +    // to avoid and may not be no-ops anyway (e.g. when some sort of mapped
> +    // memory stuff is happening).
> +    JS_ASSERT_IF(from.isFloatReg() || from.isGeneralReg(), !(from == to));

Does this same-address memory move happen?  The only case where we might expect memory-to-memory moves is in the generateVMWrapper function which copies arguments of the callVM to arguments of the C functions, and in such case they are not supposed to be identical.  In addition, I would not expect the register allocator to not produce any memory to memory move.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Does this same-address memory move happen?

I didn't check.  Do you want me to do a try push with just an assert that !(from == to)?

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Do you want me to do a try push with just an assert that !(from == to)?

I tried that, and it seems to be green: https://tbpl.mozilla.org/?tree=Try&rev=d9906f77b2a1

I guess I'll push that to inbound once the ARM bits of that patch are OKed.

Comment 17

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/515acd47156b
   https://hg.mozilla.org/integration/mozilla-inbound/rev/22310b95d586

Thanks for the reviews!

Comment 18

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/515acd47156b
https://hg.mozilla.org/mozilla-central/rev/22310b95d586