Use IonBuilder for arguments usage analysis

RESOLVED FIXED in mozilla31



5 years ago
5 years ago


(Reporter: bhackett, Assigned: bhackett)


Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(1 attachment)



5 years ago
Posted 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)
Comment on attachment 8405501 [details] [diff] [review]

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]

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


While you are there, would you also want to adjust:

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

5 years ago
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.


Comment 6

5 years ago
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).
Assignee: nobody → bhackett1024
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31


5 years ago
Blocks: 1002271
Depends on: 1003161
You need to log in before you can comment on or make changes to this bug.