Closed Bug 589932 Opened 10 years ago Closed 10 years ago

JM: fast path for >>>

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Unassigned)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 2 obsolete files)

No description provided.
Attached patch Patch v1 (obsolete) — Splinter Review
This passes trace-tests (and sjcl still works).
Attachment #468461 - Flags: review?(dvander)
Blocking JaegerPunyWins, SS crypto-tests use >>> too, have not measured though, maybe in the noise.
Status: NEW → ASSIGNED
Attached patch Patch v2 (obsolete) — Splinter Review
Implement x >>> constant-0 correctly + tests.
Attachment #468461 - Attachment is obsolete: true
Attachment #468488 - Flags: review?(dvander)
Attachment #468461 - Flags: review?(dvander)
Attachment #468488 - Flags: review?(dvander) → review+
Just checking, you didn't forget to commit this?
Sorry, Jan, indeed I did!

http://hg.mozilla.org/projects/jaegermonkey/rev/1035fdc5d714
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
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
Attached patch Patch v3Splinter Review
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.
Attachment #468488 - Attachment is obsolete: true
Attachment #469912 - Flags: review?(dvander)
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.
Attachment #469912 - Flags: review?(dvander) → review+
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...
(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.
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?
(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!
http://hg.mozilla.org/projects/jaegermonkey/rev/23706a777848
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Fixed a while ago.

http://hg.mozilla.org/mozilla-central/rev/7176e88f36eb
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.