Closed Bug 911834 Opened 8 years ago Closed 8 years ago

OdinMonkey: create phis with a type directly

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(2 files, 2 obsolete files)

Since the type of a slot can't change in asm.js and the types are known up-front, we can avoid the analysis in ApplyTypeInformation and just create the phis with the correct resultType immediately (just like what happens with all the other MIR nodes).  This saves a bit more than half a second of total asm.js compile time on a certain large asm.js app on a Nexus 4.
Attached patch remove dead store (obsolete) — Splinter Review
While I was looking, I noticed that this setSlot shouldn't be necessary.
Attachment #798611 - Flags: review?(jdemooij)
Attached patch create phis with types (obsolete) — Splinter Review
This patch simply propagates the types of predecessor MDefinitions into the MPhi.  With this change, no MIR nodes created by Odin should have MIRType_Value.  The main question is whether to add extra parameters/branches to the existing paths or whether to clone.  In two cases where I felt the extra branches might have affected perf and made the methods harder to read, I cloned and in the third case I added a branch.  Let me know if you think they'd be better the other way, though.
Attachment #798612 - Flags: review?(jdemooij)
Attachment #798613 - Flags: review?(jdemooij)
Comment on attachment 798611 [details] [diff] [review]
remove dead store

Oops, this is totally not a dead store.
Attachment #798611 - Attachment is obsolete: true
Attachment #798611 - Flags: review?(jdemooij)
Attachment #798612 - Attachment is obsolete: true
Attachment #798612 - Flags: review?(jdemooij)
Attachment #798622 - Flags: review?(jdemooij)
Comment on attachment 798613 [details] [diff] [review]
disable ApplyTypeInformation when compilingAsmJS

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

::: js/src/jit/Ion.cpp
@@ +1228,5 @@
>      if (mir->shouldCancel("Phi reverse mapping"))
>          return false;
>  
> +    if (!mir->compilingAsmJS()) {
> +        // This pass also removes copies.

Nit: can you remove this comment while you're here? Copies (MCopy) were removed a long time ago (bug 732862).
Attachment #798613 - Flags: review?(jdemooij) → review+
Comment on attachment 798622 [details] [diff] [review]
create phis with types

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

Looks good.
Attachment #798622 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/45e1927c9136
https://hg.mozilla.org/mozilla-central/rev/ed2a7239fc36
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.