1012922 - IonMonkey: Optimize away some copy instructions on x64

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: micro-scratchreg-avoidance.patch

The attached patch contains a few collected x64 micro-optimizations to use fewer copies into scratch registers.

Comment 1

[Jan de Mooij [:jandem]]

Comment on attachment 8425099 [details] [diff] [review]
micro-scratchreg-avoidance.patch

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

::: js/src/jit/shared/Assembler-x86-shared.h
@@ +114,5 @@
>      }
> +
> +    bool containsReg(Register r) const {
> +        switch (kind()) {
> +        case REG:          return r.code() == reg();

Nit: indent cases and default with 2 spaces.

@@ +117,5 @@
> +        switch (kind()) {
> +        case REG:          return r.code() == reg();
> +        case MEM_REG_DISP: return r.code() == base();
> +        case MEM_SCALE:    return r.code() == base() || r.code() == index();
> +        default: break;

I'd prefer a MOZ_ASSUME_UNREACHABLE to avoid giving the wrong answer if we ever add another type.

::: js/src/jit/x64/MacroAssembler-x64.h
@@ +455,5 @@
>  
>      void cmpPtr(Register lhs, const ImmWord rhs) {
>          JS_ASSERT(lhs != ScratchReg);
> +        if ((intptr_t)rhs.value <= INT32_MAX && (intptr_t)rhs.value >= INT32_MIN) {
> +            cmpq(lhs, Imm32((int32_t)rhs.value));

Nit: C++-style casts: intptr_t(rhs.value), int32_t(rhs.value).

@@ +465,5 @@
>      void cmpPtr(Register lhs, const ImmPtr rhs) {
>          cmpPtr(lhs, ImmWord(uintptr_t(rhs.value)));
>      }
>      void cmpPtr(Register lhs, const ImmGCPtr rhs) {
> +        cmpPtr(lhs, ImmWord(uintptr_t(rhs.value)));

For ImmGCPtr we need to call writeDataRelocation to make sure it's traced, we'll no longer do that if we convert it to ImmWord.

Comment 2

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Jan de Mooij [:jandem] from comment #1)
> I'd prefer a MOZ_ASSUME_UNREACHABLE to avoid giving the wrong answer if we
> ever add another type.

MOZ_ASSERT_UNREACHABLE (bug 990764)? It doesn't sound like you mean MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE.

Comment 3

[Dan Gohman [:sunfish]]

Attachment: micro-scratchreg-avoidance.patch

(In reply to Jan de Mooij [:jandem] from comment #1)
> Comment on attachment 8425099 [details] [diff] [review]
> micro-scratchreg-avoidance.patch
> 
> Review of attachment 8425099 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/shared/Assembler-x86-shared.h
> @@ +114,5 @@
> >      }
> > +
> > +    bool containsReg(Register r) const {
> > +        switch (kind()) {
> > +        case REG:          return r.code() == reg();
> 
> Nit: indent cases and default with 2 spaces.

Fixed.

> @@ +117,5 @@
> > +        switch (kind()) {
> > +        case REG:          return r.code() == reg();
> > +        case MEM_REG_DISP: return r.code() == base();
> > +        case MEM_SCALE:    return r.code() == base() || r.code() == index();
> > +        default: break;
> 
> I'd prefer a MOZ_ASSUME_UNREACHABLE to avoid giving the wrong answer if we
> ever add another type.

Ok. I used MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE; discussion below.

> ::: js/src/jit/x64/MacroAssembler-x64.h
> @@ +455,5 @@
> >  
> >      void cmpPtr(Register lhs, const ImmWord rhs) {
> >          JS_ASSERT(lhs != ScratchReg);
> > +        if ((intptr_t)rhs.value <= INT32_MAX && (intptr_t)rhs.value >= INT32_MIN) {
> > +            cmpq(lhs, Imm32((int32_t)rhs.value));
> 
> Nit: C++-style casts: intptr_t(rhs.value), int32_t(rhs.value).

Fixed.

> @@ +465,5 @@
> >      void cmpPtr(Register lhs, const ImmPtr rhs) {
> >          cmpPtr(lhs, ImmWord(uintptr_t(rhs.value)));
> >      }
> >      void cmpPtr(Register lhs, const ImmGCPtr rhs) {
> > +        cmpPtr(lhs, ImmWord(uintptr_t(rhs.value)));
> 
> For ImmGCPtr we need to call writeDataRelocation to make sure it's traced,
> we'll no longer do that if we convert it to ImmWord.

Ah, thanks for making me aware of that. I reverted those changes.

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #2)
> (In reply to Jan de Mooij [:jandem] from comment #1)
> > I'd prefer a MOZ_ASSUME_UNREACHABLE to avoid giving the wrong answer if we
> > ever add another type.
> 
> MOZ_ASSERT_UNREACHABLE (bug 990764)? It doesn't sound like you mean
> MOZ_MAKE_COMPILER_BELIEVE_IS_UNREACHABLE.

The comment in mfbt/Assertions.h says:

 * SpiderMonkey is a different beast, and there it's acceptable to use
 * MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE more widely.

This is in SpiderMonkey, so I used MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE. I'm happy to change it if something else would be better.

Comment 4

[Terrence Cole [:terrence]]

(In reply to Dan Gohman [:sunfish] from comment #3)
> 
> The comment in mfbt/Assertions.h says:
> 
>  * SpiderMonkey is a different beast, and there it's acceptable to use
>  * MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE more widely.
> 
> This is in SpiderMonkey, so I used MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE.
> I'm happy to change it if something else would be better.

I believe that comment is snark? MOZ_ASSUME_UNREACHABLE used to be a nice, safe MOZ_CRASH, at which point we went through SM and added it everywhere, replacing JS_ASSERT(false). Later, after we had done this, someone well meaning thought the naming meant it should expand to _builtin_unreachable: with that, adding a new enumerant silently triggers undefined behavior all over the engine.

There was a huge bikeshed argument on dev-platform about the naming awhile ago. About halfway through I realized the whole situation was dumb and have been switching things over to use MOZ_CRASH as I touch them. I'd suggest using MOZ_CRASH, unless the performance situation is really, truly dire; in which case MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE will at least warn on nightly builds. :-/

Comment 5

[Dan Gohman [:sunfish]]

Attachment: micro-scratchreg-avoidance.patch

(In reply to Terrence Cole [:terrence] from comment #4)
> (In reply to Dan Gohman [:sunfish] from comment #3)
> > 
> > The comment in mfbt/Assertions.h says:
> > 
> >  * SpiderMonkey is a different beast, and there it's acceptable to use
> >  * MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE more widely.
> > 
> > This is in SpiderMonkey, so I used MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE.
> > I'm happy to change it if something else would be better.
> 
> I believe that comment is snark?
[...]

My snark-detector was apparently not calibrated to detect this magnitude of snark in this context. Grepping through SpiderMonkey today, I don't see MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE anywhere. Here's a new patch which just uses MOZ_CRASH, as suggested here.

Comment 6

[Emanuel Hoogeveen [:ehoogeveen]]

FWIW, from past discussion on the subject I think that comment means "SpiderMonkey is such a performance sensitive area of Gecko that MOZ_ASSUME_UNREACHABLE is more widely used." I don't think it was intended as snark, but it may be that the reasoning for using MOZ_ASSUME_UNREACHABLE was miscommunicated or misinterpreted since, as Terrence said, it used to be safe.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8430440 [details] [diff] [review]
micro-scratchreg-avoidance.patch

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

Nice.

::: js/src/jit/x64/MacroAssembler-x64.h
@@ +599,5 @@
> +        if (JSC::X86Assembler::isAddressImmediate(address.addr)) {
> +            testl(Operand(address), imm);
> +        } else {
> +            mov(ImmPtr(address.addr), ScratchReg);
> +            testl(Operand(Address(ScratchReg, 0)), imm);

Nit: Operand(ScratchReg, 0)

Comment 8

[Terrence Cole [:terrence]]

(In reply to Dan Gohman [:sunfish] from comment #5)
> 
> My snark-detector was apparently not calibrated to detect this magnitude of
> snark in this context. Grepping through SpiderMonkey today, I don't see
> MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE anywhere. Here's a new patch which
> just uses MOZ_CRASH, as suggested here.

I'd guess that the vast majority of platform engineers did not actually make it to the end of that monster-thread, so if we do have better tools now it's unlikely that anyone knows or has acted on it.

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #6)
> FWIW, from past discussion on the subject I think that comment means
> "SpiderMonkey is such a performance sensitive area of Gecko that
> MOZ_ASSUME_UNREACHABLE is more widely used." I don't think it was intended
> as snark, but it may be that the reasoning for using MOZ_ASSUME_UNREACHABLE
> was miscommunicated or misinterpreted since, as Terrence said, it used to be
> safe.

I'm sorry. I was not trying to imply that anyone was being /intentionally/ snarky here, although I guess that is what my use of "snarky" in this context implies, more or less directly. I know that in the past, the SM team has itself given off a quite cavalier attitude to these sorts of code quality issues, so I do not at all take offense to seeing that attitude reflected in comments.

Comment 9

[Dan Gohman [:sunfish]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> Nit: Operand(ScratchReg, 0)

Fixed. https://hg.mozilla.org/integration/mozilla-inbound/rev/19a798e76a84

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/19a798e76a84