Closed Bug 937763 Opened 7 years ago Closed 7 years ago

getPropTryCommonGetter is emitting unnecessary moves

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: bzbarsky, Assigned: shu)

Details

Attachments

(1 file, 1 obsolete file)

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?
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.  ;)
Flags: needinfo?(shu)
Attached patch wip (obsolete) — Splinter Review
bz, can you check if this patch helps at all?
Assignee: nobody → shu
Status: NEW → ASSIGNED
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 on attachment 8335035 [details] [diff] [review]
wip

Jan, does this patch even make sense?
Attachment #8335035 - Flags: feedback?(jdemooij)
The problem is that we emit MIR marked as "isEmittedAtUses" when we lower them, regardless of whether they are used.
Flags: needinfo?(shu)
Attached patch wip v2Splinter Review
Boris, could you try this one as well?
Attachment #8335172 - Flags: review?(jdemooij)
Attachment #8335035 - Attachment is obsolete: true
Attachment #8335035 - Flags: feedback?(jdemooij)
Yes, that works too.
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?
Attachment #8335172 - Flags: review?(jdemooij)
(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 on attachment 8335172 [details] [diff] [review]
wip v2

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

Ah, makes sense. Good catch btw.
Attachment #8335172 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/551b2064b705
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.