Closed
Bug 589932
Opened 14 years ago
Closed 14 years ago
JM: fast path for >>>
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: jandem, Unassigned)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•14 years ago
|
||
This passes trace-tests (and sjcl still works).
Attachment #468461 -
Flags: review?(dvander)
Reporter | ||
Comment 2•14 years ago
|
||
Blocking JaegerPunyWins, SS crypto-tests use >>> too, have not measured though, maybe in the noise.
Blocks: JaegerPunyWins
Status: NEW → ASSIGNED
Reporter | ||
Comment 3•14 years ago
|
||
Implement x >>> constant-0 correctly + tests.
Attachment #468461 -
Attachment is obsolete: true
Attachment #468488 -
Flags: review?(dvander)
Attachment #468461 -
Flags: review?(dvander)
Updated•14 years ago
|
Attachment #468488 -
Flags: review?(dvander) → review+
Reporter | ||
Comment 4•14 years ago
|
||
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: 14 years ago
Resolution: --- → FIXED
Comment 6•14 years ago
|
||
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 → ---
Reporter | ||
Comment 7•14 years ago
|
||
(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•14 years ago
|
||
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
Reporter | ||
Comment 9•14 years ago
|
||
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
Reporter | ||
Updated•14 years ago
|
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+
Comment 11•14 years ago
|
||
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...
Reporter | ||
Comment 12•14 years ago
|
||
(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•14 years ago
|
||
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?
Reporter | ||
Comment 14•14 years ago
|
||
(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•14 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 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 → ---
Comment 17•14 years ago
|
||
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•14 years ago
|
||
Whiteboard: fixed-in-tracemonkey
Reporter | ||
Comment 19•14 years ago
|
||
Fixed a while ago.
http://hg.mozilla.org/mozilla-central/rev/7176e88f36eb
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•