Remove dependence of IonBuilder on ScriptAnalysis

RESOLVED FIXED in mozilla27

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

unspecified
mozilla27
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 806152 [details] [diff] [review]
patch

With bug 916914 done, removing the remaining dependencies that IonBuilder has on ScriptAnalysis, and using BytecodeAnalysis instead, is fairly straightforward.
Attachment #806152 - Flags: review?(jdemooij)
Comment on attachment 806152 [details] [diff] [review]
patch

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

Very nice, now we only need to port the arguments analysis to IonBuilder and we can rm ScriptAnalysis.

However, Till tried to remove the ScriptAnalysis::arrayWriteHole bit in bug 894948 and it regressed Octane-crypto. See bug 894948 comment 8 for what I *think* caused this. The easiest fix is to add a "write hole" flag to the baseline SetElem IC. We should do that and make sure it doesn't regress Octane-crypto.

::: js/src/jit/IonBuilder.cpp
@@ +7379,2 @@
>          SetElemICInspector icInspect(inspector->setElemICInspector(pc));
>          writeHole |= icInspect.sawOOBDenseWrite();

Nit: s/|=/=

::: js/src/jit/MIRGraph.h
@@ +9,5 @@
>  
>  // This file declares the data structures used to build a control-flow graph
>  // containing MIR.
>  
> +#include "jit/BytecodeAnalysis.h"

Nit: you can forward-declare BytecodeAnalysis since we only use a pointer to it.

::: js/src/vm/Interpreter.cpp
@@ +1225,5 @@
>  static JS_ALWAYS_INLINE bool
>  SetObjectElementOperation(JSContext *cx, Handle<JSObject*> obj, HandleId id, const Value &value,
>                            bool strict, JSScript *maybeScript = NULL, jsbytecode *pc = NULL)
>  {
>      RootedScript script(cx, maybeScript);

Also remove this root + the maybeScript/pc arguments.
Attachment #806152 - Flags: review?(jdemooij)
(Assignee)

Comment 2

4 years ago
Created attachment 806670 [details] [diff] [review]
updated

Good catch, that crypto regression was in place and your suggested fix fixes it.
Assignee: general → bhackett1024
Attachment #806152 - Attachment is obsolete: true
Attachment #806670 - Flags: review?(jdemooij)
(Assignee)

Comment 3

4 years ago
Created attachment 806672 [details] [diff] [review]
updated

This time without an unrelated jit-tests change.
Attachment #806670 - Attachment is obsolete: true
Attachment #806670 - Flags: review?(jdemooij)
Attachment #806672 - Flags: review?(jdemooij)

Updated

4 years ago
Attachment #806672 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 5

4 years ago
Followup to unbreak --disable-ion builds:

https://hg.mozilla.org/integration/mozilla-inbound/rev/38d8c6c2c223
https://hg.mozilla.org/mozilla-central/rev/ed91189f940e
https://hg.mozilla.org/mozilla-central/rev/38d8c6c2c223
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.