995336 - Use IonBuilder for arguments usage analysis

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

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.

Comment 1

[Luke Wagner [:luke]]

\o/

Comment 2

[Jan de Mooij [:jandem]]

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).

Comment 3

[Hannes Verschore [:h4writer]]

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...

Comment 4

[Brian Hackett [Laid off!]]

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android armv6 crashes. I tried to ping you on IRC, but you weren't around.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a02cface4e29

https://tbpl.mozilla.org/php/getParsedLog.php?id=38414136&tree=Mozilla-Inbound

Comment 6

[Brian Hackett [Laid off!]]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aa534ca9cea5