916914 - Remove ScriptAnalysis::ionInlineable()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

To be able to build MIR off thread we need to remove the dependence that IonBuilder has on ScriptAnalysis, which is created on demand only on the main thread.  One piece of this is ScriptAnalysis::ionInlineable(), which should be split off because it is almost entirely unnecessary and shouldn't have its semantics cargo culted into the baseline analysis.  We mark a lot of opcodes as not ion inlineable in the analysis, but many of these aren't even ion compileable and Ion has a mechanism to mark scripts as uninlineable if it fails to build them.  The only opcode I found which this bit was actually necessary for (via jit-tests --ion-eager) is JSOP_THROW, but there could be others related to try/catch I guess.

Comment 1

[Jan de Mooij [:jandem]]

Comment on attachment 805468 [details] [diff] [review]
patch

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

Nice, r=me with comment addressed.

::: js/src/jit/IonBuilder.cpp
@@ +3466,5 @@
>  
>  IonBuilder::ControlStatus
>  IonBuilder::processThrow()
>  {
> +    // JSOP_THROW can't be compiled within inlined frames.

I think we should also abort when inlining JSOP_TRY. JSOP_THROW is just a VM call so I'm afraid the problem here was caused by inlining a try-catch. Bailing out to Baseline is complicated enough and although we should support inlining JSOP_TRY eventually, it's better to do that as a separate patch I think (to help bisecting etc).

Comment 2

[Hannes Verschore [:h4writer]]

Comment on attachment 805468 [details] [diff] [review]
patch

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

::: js/src/jit/IonBuilder.cpp
@@ +3788,3 @@
>          if (inlineBuilder.abortReason_ == AbortReason_Disable) {
> +            calleeScript->uninlineable = true;
> +            abortReason_ = AbortReason_Inlining;

Moving this inside "if" is wrong, IIRC.
As soon as we inlining more than 1 depth:
A -> B -> C

C aborts and has AbortReason_Disable
B sets AbortReason_Inlining and sets C uninlineable
A sees B.abortReason != AbortReason_Disable, so will not propagate the AbortReason_Inlining. So will return AbortReason_Disable. And "A" will get disabled to get compiled!!!!

That's the reason the that line was outside the "if". Or we add and conditional checking for AbortReason_Inlining that propagates the error, or we move the check to the original place again...

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Hannes Verschore [:h4writer] from comment #2)
> That's the reason the that line was outside the "if". Or we add and
> conditional checking for AbortReason_Inlining that propagates the error, or
> we move the check to the original place again...

Having that line outside the |if| was broken because it would turn any failure in the inlined frame into an AbortReason_Inlining, including when an exception or OOM occurred.  I'll change the abortReason test to also check for AbortReason_Inlining.

Comment 4

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/338b88955a80

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/338b88955a80