885169 - Prefer low registers like EAX over high registers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

On some architectures, it is advantageous to prefer low registers over high registers. On x86/x64 for example, many instructions have optimized encodings for the lowest register, EAX, and many instructions require a REX prefix to access the high registers.

Currently, the implementation of getAny() in TypedRegisterSet ends up preferring the high registers.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: Before changing the default order, give clients which need a specific order a way to request it.

Comment 2

[Dan Gohman [:sunfish]]

Attachment: the main change

Comment 3

[Marty Rosenberg [:mjrosenb]]

This will also help if we ever get a thumb backend.  You can only access a small number of registers using a 16 bit instruction.

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 765157 [details] [diff] [review]
Before changing the default order, give clients which need a specific order a way to request it.

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

Sorry about the delay! Also nice you split it into the no-op patch and the actual patch.

::: js/src/ion/RegisterSets.h
@@ +697,4 @@
>  // iterates forwards, that is r0 to rn
>  template <typename T>
>  class TypedRegisterForwardIterator
>  {

There is still a typo in this functions and the code will not compile if you don't change it!

TypedRegisterForwardIterator<T> operator ++(int) {
  TypedRegisterIterator<T> old(*this);
  regset_.takeFirst();
        return old;
}

should be 

TypedRegisterForwardIterator<T> operator ++(int) {
  TypedRegisterForwardIterator<T> old(*this);
  regset_.takeFirst();
        return old;
}

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 765158 [details] [diff] [review]
the main change

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

Did you encounter this anywhere? Will we see an improvement anywhere? I.e. is it necessary for asm.js benchmarks?
So will this theoretical improvement yield some practical improvements?

Comment 6

[Dan Gohman [:sunfish]]

> ::: js/src/ion/RegisterSets.h
> @@ +697,4 @@
> >  // iterates forwards, that is r0 to rn
> >  template <typename T>
> >  class TypedRegisterForwardIterator
> >  {
> 
> There is still a typo in this functions and the code will not compile if you
> don't change it!
> 
> TypedRegisterForwardIterator<T> operator ++(int) {
>   TypedRegisterIterator<T> old(*this);
>   regset_.takeFirst();
>         return old;
> }
> 
> should be 
> 
> TypedRegisterForwardIterator<T> operator ++(int) {
>   TypedRegisterForwardIterator<T> old(*this);
>   regset_.takeFirst();
>         return old;
> }

Oops, yes. I had fixed that in my patch for bug 871811, and didn't realize this patch depended on it. I'll fix this before checking it in.

> Did you encounter this anywhere? Will we see an improvement anywhere? I.e.
> is it necessary for asm.js benchmarks?
> So will this theoretical improvement yield some practical improvements?

I saw modest improvements in code size in every benchmark I looked at with this patch. I have not specifically looked at performance with just this patch.

Comment 7

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf03279e2b50

Comment 8

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa1a42a471f

Comment 9

[Dan Gohman [:sunfish]]

The patches were reverted due to B2G regressions. I will investigate.

Comment 10

[Dan Gohman [:sunfish]]

The revert commit is here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/971b68a8f9cb

Comment 11

[Dan Gohman [:sunfish]]

I re-applied the first patch, with a fix for a bug that I introduced while juggling the patch between trees, here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/765cd0d0dc5b

The second patch, to actually switch the order, is not yet applied, as there is still some outstanding issue.

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/765cd0d0dc5b

Comment 13

[Dan Gohman [:sunfish]]

The second patch, which actually changes the order and realizes the optimization, is not yet applied, because it appears to hit multiple bugs on ARM, and it's beyond my present reach.

Comment 14

[Dan Gohman [:sunfish]]

Attachment: regorder-trampoline.patch

Comment 15

[Dan Gohman [:sunfish]]

Attachment: regorder-reg-pressure.patch

Comment 16

[Nicolas B. Pierron [:nbp]]

Comment on attachment 799661 [details] [diff] [review]
regorder-reg-pressure.patch

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

::: js/src/jit/x86/MacroAssembler-x86.h
@@ +125,5 @@
> +                movl(src.typeReg(), dest.typeReg());
> +        } else {
> +            // Swap the parts.
> +            xchgl(dest.payloadReg(), dest.typeReg());
> +        }

I think the following would be easier to read:

Register s1 = src.typeReg(), d1 = dest.typeReg(),
         s2 = src.payloadReg(), d2 = dest.payloadReg();

// Swap the order if the second source correspond to the first destination.
if (s2 == d1) {
  mozilla::Swap(s1, s2);
  mozilla::Swap(d1, d2);

  // If it corresponds again, then there is a cycle.
  if (s2 == d1) {
    JS_ASSERT(s1 == d2);
    xchgl(s1, s2);
    return;
  }
}

if (s1 != d1)
  movl(s1, d1);
if (s2 != d2)
  movl(s2, d2);

Comment 17

[Dan Gohman [:sunfish]]

Attachment: updated per review feedback

How about this?

Comment 18

[Nicolas B. Pierron [:nbp]]

Comment on attachment 799875 [details] [diff] [review]
updated per review feedback

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

::: js/src/jit/arm/MacroAssembler-arm.h
@@ +956,5 @@
>      void moveValue(const ValueOperand &src, const ValueOperand &dest) {
> +        Register s0 = src.typeReg(), d0 = dest.typeReg(),
> +                 s1 = src.payloadReg(), d1 = dest.payloadReg();
> +
> +        // Either or both of the source registers could be the same as a

nit: Either *one* or both ?

Comment 19

[Dan Gohman [:sunfish]]

(In reply to Nicolas B. Pierron [:nbp] from comment #18)
> Comment on attachment 799875 [details] [diff] [review]
> updated per review feedback
> 
> Review of attachment 799875 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/arm/MacroAssembler-arm.h
> @@ +956,5 @@
> >      void moveValue(const ValueOperand &src, const ValueOperand &dest) {
> > +        Register s0 = src.typeReg(), d0 = dest.typeReg(),
> > +                 s1 = src.payloadReg(), d1 = dest.payloadReg();
> > +
> > +        // Either or both of the source registers could be the same as a
> 
> nit: Either *one* or both ?

Either one is fine with me ;-).

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a34b197be1d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a887cc385cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/997672af6fc8

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8a34b197be1d
https://hg.mozilla.org/mozilla-central/rev/7a887cc385cb
https://hg.mozilla.org/mozilla-central/rev/997672af6fc8

Comment 21

[Phil Ringnalda (:philor)]

Looks like this broke hell out of Win64 - see https://tbpl.mozilla.org/php/getParsedLog.php?id=27427851&tree=Mozilla-Central for the massive make check failures, or https://tbpl.mozilla.org/?showall=1&tree=Date&rev=997672af6fc8 if I ever manage to get tests to actually run there, or anecdotal reports of nothing but startup crashes.

Comment 22

[Dan Gohman [:sunfish]]

I'll investigate shortly. If you're looking for a patch to revert, 997672af6fc8 is the most likely.

Comment 23

[Phil Ringnalda (:philor)]

Backed out all three in https://hg.mozilla.org/mozilla-central/rev/d35342e7bcd0 because we were running out of time before we would be shipping a totally broken nightly to several tens of thousands of people.

Comment 24

[Dan Gohman [:sunfish]]

I believe I've found the bug, or at least a bug. I prepared a patch:

https://hg.mozilla.org/try/rev/20ba516b40a4

and the try results are here:

https://tbpl.mozilla.org/?tree=Try&rev=6c2d41590952

The build seems to have succeeded, and the tests appear to be passing, but the scripts seem to think that the build failed. There's no obvious failure in the logs. Is this normal for Win64, or a sign that something's still broken?

Comment 25

[Dan Gohman [:sunfish]]

Attachment: A fix for x64 similar to the one for ARM

Here's the fix mentioned above, for review, since it does seem like a good change, regardless of what the Win64 try results mean.

Comment 26

[Phil Ringnalda (:philor)]

The secret to figuring out that sort of silent failure is to look for the line just three or four lines into the actual log that says something like "results: failure (2)" and then grep the log for "results: 2". Sometimes it'll throw you off, because there are steps that claim to fail but don't actually affect the status of the build, but in this case it'll tell you that the build thought it should be red because of a totally bogus "circular directory structure" error from msys while cleaning up before the build started. Then a further look for "results: 1" will tell you that it would have been silently orange from make check thinking it failed despite no tests thinking that they failed. That's the "normal" state right now, back to where it was.

And if you look back a few days on https://tbpl.mozilla.org/?tree=Date, you'll see that your unittest failures aren't just normal, that's way way way better than normal.

Comment 27

[Dan Gohman [:sunfish]]

I re-applied the two fixes from before, plus the new fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6b0a8afb467
https://hg.mozilla.org/integration/mozilla-inbound/rev/26006e5cefdc
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2be9740720f

This does not yet include the patch to actually change the order.

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d6b0a8afb467
https://hg.mozilla.org/mozilla-central/rev/26006e5cefdc
https://hg.mozilla.org/mozilla-central/rev/e2be9740720f

Comment 29

[Dan Gohman [:sunfish]]

My interpretation of this full try run on win64 [0] is that the final patch is now good to go. I'll probably check it in in a few days.

[0] https://tbpl.mozilla.org/?tree=Try&rev=739bd6ba4f1e

Comment 30

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf6bfb9a52aa

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf6bfb9a52aa