Closed
Bug 599247
Opened 14 years ago
Closed 14 years ago
nanojit: in Nativei386.cpp, generate d[b + i<<s] addressing modes in asm_store32()
Categories
(Core Graveyard :: Nanojit, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Whiteboard: has-patch, PACMAN, fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)
Attachments
(1 file)
25.57 KB,
patch
|
edwsmith
:
review+
|
Details | Diff | Splinter Review |
We generate d[r + r*k] addressing modes in asm_load32(), but not in asm_store32(). We should do them in asm_store32(). This will particularly help when the same pattern is both loaded and stored to, as happens in TM for SETELEM.
Assignee | ||
Comment 1•14 years ago
|
||
This patch: - Introduces the use of d(b + i<<s) addressing modes for stores, where possible. I'd appreciate careful checking of the encodings because (a) x86 encodings are gnarly, and (b) the Nativei386.cpp code is gnarly. (However, I checked and TM's trace-tests suite does exercise all of the new codegen functions.) - Makes the layout of a few switches more concise. - Renames a few variables. SunSpider results are good: --------------------------------------------------------------- | millions of instructions executed | | total | on-trace (may overestimate) | --------------------------------------------------------------- | 90.553 90.540 (------) | 46.860 46.859 (------) | 3d-cube | 38.131 37.821 (1.008x) | 25.476 25.173 (1.012x) | 3d-morph | 96.556 96.532 (------) | 42.337 42.331 (------) | 3d-raytrace | 24.647 24.646 (------) | 12.241 12.241 (------) | access-binary- | 102.935 95.720 (1.075x) | 92.712 85.507 (1.084x) | access-fannkuc | 28.997 28.978 (1.001x) | 17.182 17.182 (------) | access-nbody | 39.456 36.511 (1.081x) | 28.181 25.239 (1.117x) | access-nsieve | 7.415 7.414 (------) | 3.246 3.246 (------) | bitops-3bit-bi | 36.802 36.802 (------) | 32.519 32.519 (------) | bitops-bits-in | 15.844 15.843 (------) | 12.016 12.016 (------) | bitops-bitwise | 43.266 40.358 (1.072x) | 37.964 35.056 (1.083x) | bitops-nsieve- | 17.414 17.413 (------) | 13.242 13.242 (------) | controlflow-re | 83.142 81.685 (1.018x) | 30.418 28.990 (1.049x) | crypto-aes | 32.284 32.303 (0.999x) | 4.751 4.755 (0.999x) | crypto-md5 | 19.919 19.891 (1.001x) | 6.428 6.388 (1.006x) | crypto-sha1 | 71.105 71.076 (------) | 21.851 21.851 (------) | date-format-to | 69.854 69.842 (------) | 9.611 9.611 (------) | date-format-xp | 44.898 44.904 (------) | 31.066 31.066 (------) | math-cordic | 20.490 20.490 (------) | 6.329 6.329 (------) | math-partial-s | 22.062 22.063 (------) | 13.410 13.410 (------) | math-spectral- | 49.547 49.545 (------) | 34.585 34.585 (------) | regexp-dna | 30.034 30.032 (------) | 9.278 9.278 (------) | string-base64 | 86.358 85.955 (1.005x) | 24.652 24.260 (1.016x) | string-fasta | 114.595 114.609 (------) | 17.186 17.186 (------) | string-tagclou | 161.202 161.297 (0.999x) | 21.184 21.184 (------) | string-unpack- | 43.114 43.116 (------) | 8.448 8.448 (------) | string-validat ------- | 1390.633 1375.398 (1.011x) | 603.185 587.966 (1.026x) | all That'll translate to about 1% (4ms on AWFY). It'll probably get JM+TM below JSC on fannkuch, which is nice. V8's results are little changed, as are Kraken's (surprisingly enough, given that this helps most with array accesses).
Summary: nanojit: in Nativei386.cpp, generate d[r + r*k] addressing modes in asm_store32() → nanojit: in Nativei386.cpp, generate d[b + i<<s] addressing modes in asm_store32()
Assignee | ||
Comment 2•14 years ago
|
||
Attachment #481428 -
Flags: review?(edwsmith)
Comment 3•14 years ago
|
||
Comment on attachment 481428 [details] [diff] [review] patch (against TM 54893:b75687a3151e) getBaseIndexScale() looks right. May only be useful for x86-32/64, but so what. MODRMsib changed argument 'reg' from R to I32. checked all callers. ok. Unrelated to this patch: we could rename AllowableFlagRegs in Nativei386.h to AllowableByteRegs, then use it in several places that want EAX|ECX|EDX|EBX. I got no failures in a test run, and nothing else looks wrong. I'm marking R+ in case you want to land the patch before the weekend, and I'll finish up tomorrow anyway.
Attachment #481428 -
Flags: review?(edwsmith) → review+
Assignee | ||
Comment 4•14 years ago
|
||
(In reply to comment #3) > > MODRMsib changed argument 'reg' from R to I32. checked all callers. ok. Now that Register is non-numeric in debug builds that change is type-checked by the compiler :) > Unrelated to this patch: we could rename AllowableFlagRegs in Nativei386.h to > AllowableByteRegs, then use it in several places that want EAX|ECX|EDX|EBX. Good idea. I made that change as well. > I got no failures in a test run, and nothing else looks wrong. I'm marking R+ > in case you want to land the patch before the weekend, and I'll finish up > tomorrow anyway. ... I worked out why Kraken didn't benefit from this patch -- because it only helps with arrays-of-ints, and Kraken is full of arrays-of-doubles. We'll need to do the same trick in asm_load64() and asm_store64(). I opened bug 602765 for that.
Assignee | ||
Comment 5•14 years ago
|
||
In comment 4, where I wrote "..." I meant to write this: http://hg.mozilla.org/projects/nanojit-central/rev/cf8d0148a91d
Whiteboard: fixed-in-nanojit
Assignee | ||
Comment 6•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/c7408eb9023e
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
Comment 7•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/c7408eb9023e
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 8•14 years ago
|
||
Comment on attachment 481428 [details] [diff] [review] patch (against TM 54893:b75687a3151e) http://hg.mozilla.org/tamarin-redux/rev/14625451b592
Updated•14 years ago
|
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → has-patch, PACMAN, fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
Updated•10 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•