Closed Bug 817943 Opened 7 years ago Closed 7 years ago

Improve register allocation in Ionmonkey's specialized DOM stuff

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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?
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.  ;)
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 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.
(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.
> 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...
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?
Flags: needinfo?(nicolas.b.pierron)
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.
Attachment #695109 - Flags: review?(nicolas.b.pierron) → review+
Flags: needinfo?(nicolas.b.pierron)
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.
Attachment #695110 - Flags: review?(nicolas.b.pierron) → review+
(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.
This passes tests on try, so the assert isn't triggering...
Attachment #695554 - Flags: review?(nicolas.b.pierron)
Attachment #695554 - Flags: review?(mrosenberg)
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.
Attachment #695554 - Flags: review?(nicolas.b.pierron) → review+
> 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)?
> 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.
Attachment #695554 - Flags: review?(mrosenberg) → review+
   https://hg.mozilla.org/integration/mozilla-inbound/rev/515acd47156b
   https://hg.mozilla.org/integration/mozilla-inbound/rev/22310b95d586

Thanks for the reviews!
Assignee: general → bzbarsky
Flags: in-testsuite-
Target Milestone: --- → mozilla20
Depends on: 825365
You need to log in before you can comment on or make changes to this bug.