Closed Bug 1069307 Opened 5 years ago Closed 5 years ago

Turn Scalar Replacement on by default.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: nbp, Assigned: nbp)

References

Details

(Keywords: feature)

Attachments

(2 files, 1 obsolete file)

This bug is for turning on Scalar Replacement and filing security issues which have to be fixed before landing this patch.

Scalar replcamenet can be turn on by either by applying the current patch, or by running the JavaScript shell with the  --ion-scalar-replacement=on  CLI option.
Attachment #8491471 - Flags: feedback?(gary)
Attachment #8491471 - Flags: feedback?(choller)
Depends on: 1071084
Comment on attachment 8491471 [details] [diff] [review]
IonMonkey: Enable scalar replacement by default.

I'm hitting multiple assertions, but they all eventually reduce to the same one. I've included three tests that hit this assertion because I wasn't sure if it's really all the same bug. Let me know when this one is fixed, so I can rerun the testing :)



Assertion failure: iter->type() != MIRType_None, at jit/IonAnalysis.cpp:1839
Options: --fuzzing-safe --ion-scalar-replacement=on
Test:

function init() {
    while ( {}, this) !(Object === "Infinity");      
}
eval("init()");

====


Assertion failure: iter->type() != MIRType_None, at jit/IonAnalysis.cpp:1839
Options: --fuzzing-safe --no-threads --ion-scalar-replacement=on --ion-eager
Tests:

function testCallProtoMethod() {
    var a = [new X, new X, __proto__, new new this, new Y];
}
testCallProtoMethod();

====

function testNot() {
    var r;
    for (var i = 0; i < 10; ++i)
        r = [];
}
testNot();
Attachment #8491471 - Flags: feedback?(choller) → feedback-
Comment on attachment 8491471 [details] [diff] [review]
IonMonkey: Enable scalar replacement by default.

I've been busy lately, so clearing feedback? since :decoder has a feedback-.
Attachment #8491471 - Flags: feedback?(gary)
Depends on: 1072911
Blocks: 1014649
Comment on attachment 8491471 [details] [diff] [review]
IonMonkey: Enable scalar replacement by default.

Asking for feedback again since Bug 1007291 is now riding the train on inbound, and soon on master.
Attachment #8491471 - Flags: feedback?(gary)
Attachment #8491471 - Flags: feedback?(choller)
Attachment #8491471 - Flags: feedback-
(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> Asking for feedback again since Bug 1007291 is now riding the train on
> inbound, and soon on master.

I guess you mean bug 1072911? (might start testing inbound with the patch in comment 0)
Also, I think you might need to refresh your patch here since the following checkin likely changed stuff:

$ hg log -p -r ad1b182f0f51
changeset:   206148:ad1b182f0f51
user:        Marty Rosenberg <mrosenberg@mozilla.com>
date:        Fri Sep 19 07:41:08 2014 -0400
summary:     bug 1068857: Allow overriding almost any of the jit options via the UNIX environment. (r=nbp)

Nonetheless the patch is trivial enough for me to make this change manually:

$ hg diff
diff --git a/js/src/jit/JitOptions.cpp b/js/src/jit/JitOptions.cpp
--- a/js/src/jit/JitOptions.cpp
+++ b/js/src/jit/JitOptions.cpp
@@ -62,17 +62,17 @@ JitOptions::JitOptions()
     // Whether to enable extra code to perform dynamic validation of
     // RangeAnalysis results.
     SET_DEFAULT(checkRangeAnalysis, false);

     // Whether Ion should compile try-catch statements.
     SET_DEFAULT(compileTryCatch, true);

     // Toggle whether eager scalar replacement is globally disabled.
-    SET_DEFAULT(disableScalarReplacement, true); // experimental
+    SET_DEFAULT(disableScalarReplacement, false);

     // Toggle whether global value numbering is globally disabled.
     SET_DEFAULT(disableGvn, false);

     // Toggles whether loop invariant code motion is globally disabled.
     SET_DEFAULT(disableLicm, false);

     // Toggles whether inlining is globally disabled.
Depends on: 1073722
You don't need any patch for testing, just use the shell flag :)
Comment on attachment 8491471 [details] [diff] [review]
IonMonkey: Enable scalar replacement by default.

do {
    ({
        b: a
    })
} while (x)


$ ./js-dbg-opt-64-dm-nsprBuild-linux-82d07982831c-1069307-c5-d32d700d4756 --ion-eager --no-threads w6107-reduced.js
Assertion failure: !succ->phisEmpty(), at jit/ScalarReplacement.cpp:340

and crashes js opt shell at replaceOperand.

Debug configure options:

AR=ar sh /home/fuzz2lin/trees/mozilla-inbound/js/src/configure --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests
Attachment #8491471 - Flags: feedback?(gary) → feedback-
Depends on: 1074833
Comment on attachment 8491471 [details] [diff] [review]
IonMonkey: Enable scalar replacement by default.

mozilla-central rev 2ae57957e4bb


Assertion failure: !succ->phisEmpty(), at jit/ScalarReplacement.cpp:340

Options: --ion-scalar-replacement=on
Test:

for (var i =0; 1; i++)
  ({ set: Math.w });



Assertion failure: !hasFlags(1 << Discarded), at jit/MIR.h:561

Options: --ion-scalar-replacement=on --ion-offthread-compile=off --ion-eager
Test:

test('\
var items = new Array( "one", "two", "three" );\
var itemsRef = items;\
if(itemsRef[1])\
  (itemsRef[1])\
');
test('');
function test(x) {
try { new Function(x)(); } catch(exc1) {}
}
Attachment #8491471 - Flags: feedback?(choller) → feedback-
Depends on: 1075488
Depends on: 1075601
Depends on: 1075638
Depends on: 1077041
Saw another one here, that looks different:


Assertion failure: iter->isInterruptCheck() || iter->isInterruptCheckPar(), at jit/shared/CodeGenerator-shared.cpp:1257


Options: --fuzzing-safe --thread-count=2 --ion-offthread-compile=off --ion-eager --ion-check-range-analysis --ion-regalloc=backtracking --ion-scalar-replacement=on
Test:

new Function("\
var o1 = {};\
  o1.a = 3;\
var o5 = { a: 2 };\
for (var i = 0; i < 5; i++)\
  o5.a = 3;\
var o6 = Object.preventExtensions({ a: 2 });\
for (var i = 0; i < 5; i++)\
  o6.a = 3;\
")();
Depends on: 1077427
The last remaining patches landed on inbound, it would be good for fuzzing
after the next merge (likely tonight or tomorrow).
Attachment #8491471 - Attachment is obsolete: true
Attachment #8499653 - Flags: review?(jdemooij)
Attachment #8499653 - Flags: feedback?(gary)
Attachment #8499653 - Flags: feedback?(choller)
Comment on attachment 8499653 [details] [diff] [review]
Enable scalar replacement by default.

for (var j = 0; j < 1; j++) {
    (function() {
        [Math.fround(0), ""]
    })()
}

$ ./js-dbg-opt-64-dm-nsprBuild-darwin-138e0dba47e8 --ion-scalar-replacement=on --no-threads --ion-eager testcase.js
Assertion failure: consumer->isConsistentFloat32Use(use.use()), at /Users/skywalker/trees/mozilla-inbound/js/src/jit/IonAnalysis.cpp:1369
Segmentation fault: 11

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-inbound/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests
Attachment #8499653 - Flags: feedback?(gary) → feedback-
Depends on: 1078696
Comment on attachment 8499653 [details] [diff] [review]
Enable scalar replacement by default.

Haven't seen more failures so far. feedback+ for now :)
Attachment #8499653 - Flags: feedback?(choller) → feedback+
Comment on attachment 8499653 [details] [diff] [review]
Enable scalar replacement by default.

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

Nice! Might want to give it a few more fuzzing days or wait for the merge, but we can also disable it on Aurora if we see any problems.
Attachment #8499653 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/baf095348b7e
Assignee: nobody → nicolas.b.pierron
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Does this require any sort of manual testing?
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #18)
> Does this require any sort of manual testing?

No, fuzzers are already doing a lot of automated testing way faster than any manual testing.
Thank you Nicolas!
Flags: qe-verify-
Comment on attachment 8499653 [details] [diff] [review]
Enable scalar replacement by default.

Scalar replacement has landed turned on by default, so setting feedback+ and other issues will be filed as follow-up bugs.
Attachment #8499653 - Flags: feedback- → feedback+
Depends on: 1112632
Duplicate of this bug: 856533
You need to log in before you can comment on or make changes to this bug.