Closed Bug 995336 Opened 10 years ago Closed 10 years ago

Use IonBuilder for arguments usage analysis

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
It is reasonably straightforward to convert the arguments usage analysis to use IonBuilder, in the same fashion as the definite properties analysis; the main wrinkle is that we need to build the script without having a baseline script, as baseline needs the needsArgsObj() info to correctly compile the script.  The attached patch makes this change, and also removes TypeScript::analysis, so that after this patch ScriptAnalysis will be dead code and can be removed.
Attachment #8405501 - Flags: review?(jdemooij)
\o/
Comment on attachment 8405501 [details] [diff] [review]
patch

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

Really nice! r=me with comments below addressed.

I was wondering, do we still need to Baseline-compile first for the definite-prop analysis?

::: js/src/jit/BaselineCompiler.cpp
@@ +2962,5 @@
>      Label done;
> +
> +    // Note: we will not have analyzed arguments usage in the script if this is
> +    // being done as part of the arguments usage analysis.
> +    if (!script->argumentsHasVarBinding() || (script->analyzedArgsUsage() && !script->needsArgsObj())) {

We don't Baseline-compile before the arguments analysis, so I think you can assert script->analyzedArgsUsage() here and remove it from the condition (and remove the comment).

::: js/src/jit/IonAnalysis.cpp
@@ +2293,5 @@
> +    /* arguments.length length can read fp->numActualArgs() directly. */
> +    if (ins->isCallGetProperty() && index == 0 && ins->toCallGetProperty()->name() == cx->names().length)
> +        return true;
> +
> +    printf("FAIL %s:%d\n", script->filename(), script->lineno());

Remove printf (and change comments in this function to // while you're here).

::: js/src/jit/IonBuilder.cpp
@@ +9580,5 @@
>          current->pushSlot(info().thisSlot());
>          return true;
>      }
>  
> +    // If we are doing an analysis, we don't yet know the. Instead of bailing

Nit: finish the first sentence.

::: js/src/jsinfer.cpp
@@ +1910,5 @@
>  
>      /*
>       * If this is an array initializer nested in another array initializer,
>       * try to reuse the type objects from earlier elements to avoid
>       * distinguishing elements of the outer array unnecessarily.

Also remove this comment.

Why are we removing this btw? I think this helped some SS benchmark some time ago (3d-cube?), if there's no difference there this is probably fine...

::: js/src/jsinfer.h
@@ -1228,5 @@
>  {
>      friend class ::JSScript;
>  
> -    /* Analysis information for the script, cleared on each GC. */
> -    analyze::ScriptAnalysis *analysis;

Pretty cool, TypeScript itself is empty now, except for the TypeSets that are allocated after it. I wonder if we could refactor it somehow, though TypeScript is still a nice way to access these TypeSets.

::: js/src/vm/Interpreter.cpp
@@ +2873,3 @@
>          goto error;
> +#else
> +    JS_ASSERT(script->analyzedArgsUsage());

Why does this assert hold? If it doesn't, we could just call setNeedsArgsObj(true).
Attachment #8405501 - Flags: review?(jdemooij) → review+
Comment on attachment 8405501 [details] [diff] [review]
patch

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

Nice!

While you are there, would you also want to adjust:
http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonBuilder.cpp?from=ionbuilder.cpp#621

Into 3 different spews:
1) Recompiling (like we have now)
2) Compiling (when !executionModeIsAnalysis)
3) Analyzing (when executionModeIsAnalysis)

So split the current "analyzing" into "analyzing" and "compiling". I forgot to do that when I splitted Recompiling and Analyzing...
The array initializer code was pretty much inactive since it only triggered if the script had been analyzed and before this patch we were only analyzing scripts when they used arguments.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b365c68c8a9

Try: https://tbpl.mozilla.org/?tree=Try&rev=3b97a4609ad1
Sorry, I inadvertently pushed an older version of the patch (the Android crashes are due to some compiler bug I worked around by not inlining some TypeSet methods).

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa534ca9cea5
https://hg.mozilla.org/mozilla-central/rev/aa534ca9cea5
Assignee: nobody → bhackett1024
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Blocks: 1002271
Depends on: 1003161
You need to log in before you can comment on or make changes to this bug.