Closed Bug 917441 Opened 6 years ago Closed 6 years ago

Remove dependence of IonBuilder on ScriptAnalysis

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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)
Attached patch updated (obsolete) — Splinter Review
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)
Attached patch updatedSplinter Review
This time without an unrelated jit-tests change.
Attachment #806670 - Attachment is obsolete: true
Attachment #806670 - Flags: review?(jdemooij)
Attachment #806672 - Flags: review?(jdemooij)
Attachment #806672 - Flags: review?(jdemooij) → review+
Followup to unbreak --disable-ion builds:

https://hg.mozilla.org/integration/mozilla-inbound/rev/38d8c6c2c223
Depends on: 924538
You need to log in before you can comment on or make changes to this bug.