Closed Bug 885169 Opened 7 years ago Closed 7 years ago

Prefer low registers like EAX over high registers

Categories

(Core :: JavaScript Engine, enhancement)

x86_64
All
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: sunfish, Assigned: sunfish)

References

Details

Attachments

(5 files, 1 obsolete file)

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.
Attached patch the main changeSplinter Review
Attachment #765158 - Flags: review?(hv1989)
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 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;
}
Attachment #765157 - Flags: review?(hv1989) → review+
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?
Attachment #765158 - Flags: review?(hv1989) → review+
> ::: 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.
The patches were reverted due to B2G regressions. I will investigate.
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.
https://hg.mozilla.org/mozilla-central/rev/765cd0d0dc5b
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
Keywords: helpwanted
Attachment #799658 - Flags: review?(nicolas.b.pierron)
Attached patch regorder-reg-pressure.patch (obsolete) — Splinter Review
Attachment #799661 - Flags: review?(nicolas.b.pierron)
Keywords: helpwanted
Attachment #799658 - Flags: review?(nicolas.b.pierron) → review+
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);
Attachment #799661 - Flags: review?(nicolas.b.pierron)
How about this?
Attachment #799661 - Attachment is obsolete: true
Attachment #799875 - Flags: review?(nicolas.b.pierron)
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 ?
Attachment #799875 - Flags: review?(nicolas.b.pierron) → review+
(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
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.
I'll investigate shortly. If you're looking for a patch to revert, 997672af6fc8 is the most likely.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
Flags: needinfo?(philringnalda)
Here's the fix mentioned above, for review, since it does seem like a good change, regardless of what the Win64 try results mean.
Attachment #800571 - Flags: review?(nicolas.b.pierron)
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.
Flags: needinfo?(philringnalda)
Depends on: 913272
Attachment #800571 - Flags: review?(nicolas.b.pierron) → review+
Whiteboard: [leave open]
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
Assignee: general → sunfish
https://hg.mozilla.org/mozilla-central/rev/bf6bfb9a52aa
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.