1381423 - Two small codegen improvements for visitCallGeneric and RangePopFront

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[André Bargull [:anba]]

http://searchfox.org/mozilla-central/rev/01d27fdd3946f7210da91b18fcccca01d7324fe2/js/src/jit/CodeGenerator.cpp#4173-4174 could be replaced with branchTestObjClass, :jandem said in bug 1372081, comment #3, branchTestObjClass should be preferred when we only need to test for a class once.

http://searchfox.org/mozilla-central/rev/01d27fdd3946f7210da91b18fcccca01d7324fe2/js/src/jit/CodeGenerator.cpp#6586,6600 increments |i| before entering the seek-loop and at the end of the loop body. Instead of two increments, we can simply increment |i| at the start of the loop body.

Comment 1

[André Bargull [:anba]]

Attachment: bug1381423.patch

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8886978 [details] [diff] [review]
bug1381423.patch

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

Good finds.

Comment 3

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d929d586209513f21fe082c7d03f2e4e97c43e68

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aeb1787900b8
Generate slightly better code for visitCallGeneric and RangePopFront. r=jandem

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa6ecfc06517
Generate slightly better code for visitCallGeneric and RangePopFront. r=jandem

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This is a reland of an accidental backout which pulsebot will report soon.

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac2934d4263e
Backed out changeset aeb1787900b8 for failing devtools/client/webconsole/test/browser_webconsole_output_05.js. r=backout

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for real for failing devtools' browser_dbg_variables-view-large-array-buffer.js on Windows 7 debug:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f8946e2012d00ab270726f83cdb9aa6bd1e4735

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=aa6ecfc0651723beb1afb51cb0594cb1429ee432&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=115294834&repo=mozilla-inbound

16:33:09     INFO -  539 INFO TEST-PASS | devtools/client/debugger/test/mochitest/browser_dbg_variables-view-large-array-buffer.js | The page #3 in the 'largeMap' is named correctly. -
16:33:09     INFO -  540 INFO TEST-PASS | devtools/client/debugger/test/mochitest/browser_dbg_variables-view-large-array-buffer.js | The page #3 in the 'largeMap' should not have a corresponding value. -
16:33:09     INFO -  Buffered messages finished
16:33:09    ERROR -  541 INFO TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg_variables-view-large-array-buffer.js | The page #4 in the 'largeMap' is named correctly. - Got [7500…9998], expected [7500…9999]
16:33:09     INFO -  Stack trace:
16:33:09     INFO -  chrome://mochikit/content/browser-test.js:test_is:967
16:33:09     INFO -  chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/browser_dbg_variables-view-large-array-buffer.js:verifyFirstLevel/</<:160
16:33:09     INFO -  chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/browser_dbg_variables-view-large-array-buffer.js:verifyFirstLevel/<:157
16:33:09     INFO -  resource://devtools/shared/deprecated-sync-thenables.js:resolve:42
16:33:09     INFO -  resource://devtools/shared/deprecated-sync-thenables.js:then:22
16:33:09     INFO -  resource://devtools/shared/deprecated-sync-thenables.js:resolve:77
16:33:09     INFO -  chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/browser_dbg_variables-view-large-array-buffer.js:getExpandedPages/<:133
16:33:09     INFO -  resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:process:922
16:33:09     INFO -  resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:walkerLoop:806

Comment 9

[André Bargull [:anba]]

Attachment: bug1381423-branch-notequal.patch

Hi Jan,

it seems like the original patch led to issues on 32-bit builds (comment #8). I was able to reproduce the bug on a linux32 shell with this minimal test case:

  var m = new Map(Array(10000).fill(0).map((v, k) => [k + 1, null]));
  do {
    var n = [...m].length;
    if (n !== 10000)
      print("bad length:", n);
  } while(!inIon());

It is possible to fix this issue by reverting the masm.branchTestMagic change in RangePopFront (bug1381423-branch-notequal.patch contains this change):

-    masm.branchTestMagic(Assembler::Equal, Address(front, OrderedHashTable::offsetOfEntryKey()),
-                         JS_HASH_KEY_EMPTY, &seek);
+    masm.branchTestMagic(Assembler::NotEqual, Address(front, OrderedHashTable::offsetOfEntryKey()),
+                         JS_HASH_KEY_EMPTY, &done);
+    masm.jump(&seek);


But I don't really understand why reverting these lines fixes the bug. I'd assume |masm.branchTestMagic(Assembler::Equal, ..., &seek)| is equivalent to the other code with |masm.branchTestMagic| and the extra |masm.jump|. Can you help me out to understand why reverting the branching logic fixes the 32-bit bug?

Thanks,
André

Comment 10

[André Bargull [:anba]]

Uh, it seems like |MacroAssembler::branchTestMagic(Condition, const Address&, JSWhyMagic, Label*)| is broken on 32-bits builds [1] when |cond == Equal|. The first check directly jumps to the label when the value is magic for |cond == Equal|, but it doesn't check the magic-kind.

[1] http://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/js/src/jit/x86/MacroAssembler-x86-inl.h#918-923
[2] http://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/js/src/jit/x86/MacroAssembler-x86-inl.h#921

Comment 11

[André Bargull [:anba]]

Attachment: bug1381423-part0-branchtestmagic-32bit.patch

MacroAssembler::branchTestMagic(Condition, const Address&, JSWhyMagic, Label*) on 32-bit targets didn't handle the |cond == Assember::Equal| case correctly.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8887863 [details] [diff] [review]
bug1381423-part0-branchtestmagic-32bit.patch

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

Ah good find.

Comment 13

[André Bargull [:anba]]

Attachment: bug1381423-part1-codegen.patch

Updated the original patch to include a regression test case based on the reduced test case from comment #9. Otherwise no changes, therefore carrying r+ from :jandem.

Comment 14

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cb7ac87f0fb262adae3c9e07de41471bb5203b9

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba918d2bbf46
Part 0: Fix branchTestMagic with an Address operand for 32-bit targets. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/e52913f18124
Part 1: Generate slightly better code for visitCallGeneric and RangePopFront. r=jandem

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ba918d2bbf46
https://hg.mozilla.org/mozilla-central/rev/e52913f18124