937763 - getPropTryCommonGetter is emitting unnecessary moves

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Testcase:

  var d = document.createElement("canvas").getContext("2d").createImageData(1, 1);
  var count = 10000000;
  var z;
  for (var i = 0; i < count; ++i) {
    z = d.data;
  }

and JIT Inspector shows the output as:

[LoadSlotT]
movabsq    $0x7fffffffffff, %r11
movq       0x6a0(%rax), %rsi
andq       %r11, %rsi
[Pointer:GC_THING]
movabsq    $0x11c23c160, %rax
[GuardShape]
movabsq    $0x11c24b358, %r11
cmpq       %r11, 0(%rax)
jne        ((1941))
[Pointer:GC_THING]
movabsq    $0x11c23c160, %rax
[MoveGroup]
[GetDOMProperty]
movabsq    $0xfff9000000000000, %r11
push       %r11
movq       %rsp, %rcx
push       %rsi
movq       0x20(%rsi), %rdx
shlq       $1, %rdx
movq       %rsp, %rsi
movabsq    $0xffffffffffffffff, %rdi
push       $0x700
push       %rdi
movabsq    $0x1086730d0, %r11
movq       %rsp, 0x0(%r11)
push       $0x1
push       $0x0
movq       %rsp, %rdi
andq       $0xfffffff0, %rsp
push       %rdi
movabsq    $0x1086730d8, %r11
movq       0x0(%r11), %rdi
subq       $0x8, %rsp
movabsq    $0x103a6fa50, %rax

etc.  So this loads some JSObject* into %rsi, then does a shape guard on 0x11c23c160 (which is the foundProto in getPropTryCommonGetter), then loads that proto's constant value into %rax again.... and then never reads %rax before writing to it.

So why do we need the second "movabsq    $0x11c23c160, %rax", exactly?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, and the reason I care about this is that with the patches in bug 935855 I get the above code for d.data looking more like:

[LoadSlotT]
movabsq    $0x7fffffffffff, %r11
movq       0x6a0(%rax), %rcx
andq       %r11, %rcx
[Pointer:GC_THING]
movabsq    $0x10feaa790, %rax
[GuardShape]
movabsq    $0x10eaf1b00, %r11
cmpq       %r11, 0(%rax)
jne        ((2018))
[Pointer:GC_THING]
movabsq    $0x10feaa790, %rax
[GetDOMMember]
movq       0x38(%rcx), %rax
[Unbox:Object]
movabsq    $0x7fffffffffff, %r11
andq       %r11, %rax
[TypeBarrierO]
movabsq    $0x10ead5f40, %r11
cmpq       %r11, %rax
jne        ((2064))

so now the extra register mov might even be measurable.  ;)

Comment 2

[Shu-yu Guo [:shu]]

Attachment: wip

bz, can you check if this patch helps at all?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yes, that makes the problem go away.  Now I get:

[LoadSlotT]
movabsq    $0x7fffffffffff, %r11
movq       0x6a0(%rax), %rcx
andq       %r11, %rcx
[Pointer:GC_THING]
movabsq    $0x10b34b520, %rax
[GuardShape]
movabsq    $0x12045f830, %r11
cmpq       %r11, 0(%rax)
jne        ((1652))
[GetDOMMember]
movq       0x38(%rcx), %rax
[Unbox:Object]
movabsq    $0x7fffffffffff, %r11
andq       %r11, %rax
[TypeBarrierO]
movabsq    $0x11c3ced00, %r11
cmpq       %r11, %rax
jne        ((1688))

without the extra movabsq.

Comment 4

[Shu-yu Guo [:shu]]

Comment on attachment 8335035 [details] [diff] [review]
wip

Jan, does this patch even make sense?

Comment 5

[Shu-yu Guo [:shu]]

The problem is that we emit MIR marked as "isEmittedAtUses" when we lower them, regardless of whether they are used.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: wip v2

Boris, could you try this one as well?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yes, that works too.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8335172 [details] [diff] [review]
wip v2

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

::: js/src/jit/shared/Lowering-shared-inl.h
@@ +182,5 @@
> +    if (as->isEmittedAtUses() &&
> +        (def->type() == as->type() ||
> +         (as->isConstant() &&
> +          (def->type() == MIRType_Int32 || def->type() == MIRType_Boolean) &&
> +          (as->type() == MIRType_Int32 || as->type() == MIRType_Boolean))))

Can you explain why we need this? What breaks if we don't have this here?

Comment 9

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> Comment on attachment 8335172 [details] [diff] [review]
> wip v2
> 
> Review of attachment 8335172 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/shared/Lowering-shared-inl.h
> @@ +182,5 @@
> > +    if (as->isEmittedAtUses() &&
> > +        (def->type() == as->type() ||
> > +         (as->isConstant() &&
> > +          (def->type() == MIRType_Int32 || def->type() == MIRType_Boolean) &&
> > +          (as->type() == MIRType_Int32 || as->type() == MIRType_Boolean))))
> 
> Can you explain why we need this? What breaks if we don't have this here?

There are a bunch of places where we redefine conversions of constants as their constant value. The only case where we do this even when the MIRTypes don't match is if int32<->bool conversions. For example, http://dxr.mozilla.org/mozilla-central/source/js/src/jit/Lowering.cpp#1742

Both boolean and int32 constants get emitted as LInteger or somesuch, but if we stay at the MIR level, you start running into MIRType mismatch assertions if you just replace the MIRs.

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8335172 [details] [diff] [review]
wip v2

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

Ah, makes sense. Good catch btw.

Comment 11

[Shu-yu Guo [:shu]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/551b2064b705

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/551b2064b705