589932 - JM: fast path for >>>

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch v1

This passes trace-tests (and sjcl still works).

Comment 2

[Jan de Mooij [:jandem]]

Blocking JaegerPunyWins, SS crypto-tests use >>> too, have not measured though, maybe in the noise.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch v2

Implement x >>> constant-0 correctly + tests.

Comment 4

[Jan de Mooij [:jandem]]

Just checking, you didn't forget to commit this?

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Sorry, Jan, indeed I did!

http://hg.mozilla.org/projects/jaegermonkey/rev/1035fdc5d714

Comment 6

[Sean Stangl [:sstangl]]

Backed out push -- tree was on fire, and trace-tests fail. Something the patch depends on probably changed in the brief period between review and push.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to comment #6)
> Backed out push -- tree was on fire, and trace-tests fail. Something the patch
> depends on probably changed in the brief period between review and push.

Yeah, but that was not my patch. Diff looks more like the scripted-IC one.

Comment 8

[Sean Stangl [:sstangl]]

Applying the patch in the file yields the following errors on a debug build of x64:

[sean@moogle src]$ python trace-test/trace-test.py --jitflags=m dbg64/js
[ 815|   3| 818] 100% ===============================================>|   24.3s
FAILURES:
    -m /home/sean/dev/jaegermonkey/js/src/trace-test/tests/sunspider/check-crypto-aes.js
    -m /home/sean/dev/jaegermonkey/js/src/trace-test/tests/sunspider/check-crypto-md5.js
    -m /home/sean/dev/jaegermonkey/js/src/trace-test/tests/sunspider/check-crypto-sha1.js

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Patch v3

Thanks. I used a release build for the trace-tests, will use debug next time. The attached patch changes ownRegForData to copyDataIntoReg. Passes trace-tests in debug and release mode.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 469912 [details] [diff] [review]
Patch v3

Sorry for not catching this earlier. ownRegForData is fairly violent and should go away at some point.

Comment 11

[Sean Stangl [:sstangl]]

Note that patch v3 has the same incorrect isNotType() usage as in bug 593554. Since everyone seems to be getting this wrong, maybe it's best to change the definition...

Comment 12

[Jan de Mooij [:jandem]]

(In reply to comment #11)
> Note that patch v3 has the same incorrect isNotType() usage as in bug 593554.

The use of isNotType here predates this patch. Anyway, I don't think it's incorrect here. We check if one of the types is definitely known to be != int32 and then emit a stub call. After this, the type is either unknown, or known to be int32. If it's unknown, we emit a runtime check.

But I agree that it can be confusing and error-prone. We could also look how JSC and V8 handle this.

Comment 13

[Sean Stangl [:sstangl]]

Ah, my mistake -- I just glanced at the diff, which contained the comment "We only want to handle integers here" above the isNotType(JSVAL_TYPE_INT32) checks. I misread that as the author expecting the isNotType() checks to rule out everything but int32.

The usage should then be fine. Is there any reason this hasn't been pushed?

Comment 14

[Jan de Mooij [:jandem]]

(In reply to comment #13)
> Is there any reason this hasn't been pushed?

Not that I know of, we just need somebody to commit it -- hint hint!

Comment 15

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/projects/jaegermonkey/rev/23706a777848

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

JM tree is defunct(-ish), you want to land on TM and set whiteboard to fixed-in-tracemonkey.

isNotType is kind of confusing, indeed. It's really "isDefinitelyNotType" or "isKnownAsNotType", but those are kind of long.

Comment 17

[Brendan Eich [:brendan]]

It would be better to penalize the lower-level method with the longer name, and make an isNotType that handles the indefinite case by returning false.

/be

Comment 18

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/tracemonkey/rev/7176e88f36eb

Comment 19

[Jan de Mooij [:jandem]]

Fixed a while ago.

http://hg.mozilla.org/mozilla-central/rev/7176e88f36eb