Closed
Bug 1316848
Opened 8 years ago
Closed 7 years ago
Wasm baseline: Avoid using a zero temp register for EqzI64
Categories
(Core :: JavaScript Engine: JIT, defect, P3)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: lth, Assigned: away)
References
Details
Attachments
(1 file, 1 obsolete file)
1.71 KB,
patch
|
lth
:
review+
|
Details | Diff | Splinter Review |
Currently we set a temp register to zero and then call cmp64Set on the two registers and a target, this is pretty wasteful code. Since cmp64Set is unlikely to be optimized away (see bug 1316822) there's reason to avoid it here; after all, comparing to zero is a very simple matter, and requires different code, than comparing two 64-bit values.
I feel slightly dirty about the 32-bit codepath, because the nearby functions seem to prefer using higher-level abstractions. Is it ok to go into masm specifics here? wasm/spec/i64.wast has good coverage for all four interesting cases ("high" and "low" being zero and non-zero). I also did a quick verify that the x86 comparison against Imm32(0) indeed boils down to a "test".
Attachment #8813427 -
Flags: review?(lhansen)
Reporter | ||
Comment 2•7 years ago
|
||
Comment on attachment 8813427 [details] [diff] [review] eqzi64 Review of attachment 8813427 [details] [diff] [review]: ----------------------------------------------------------------- There's a general rule that we do not mix value stack management and platform-specific computation, see comment block at line 3505. This keeps platform code pretty well isolated in one part of the file. As a consequence you'll want to factor out something like void eqz64(RegI64 src, RegI32 dest) which could reasonably have the additional requirement that src==dest on 64-bit systems and src.low == dest on 32-bit systems, anyway that function will hide the platform-specific bits. ::: js/src/wasm/WasmBaselineCompile.cpp @@ +4433,5 @@ > + > +#if defined(JS_CODEGEN_X64) > + masm.cmpq(Imm32(0), r0.reg); > + masm.emitSet(Assembler::Equal, i0); > +#elif defined(JS_CODEGEN_X86) || defined(JS_CODEGEN_ARM) In the 32-bit case you should be able to avoid the jump and label song and dance by or'ing the high and low together and testing that directly, I think. In this case, the high register is even free. or(low, high) // low==i0 cmp(high, 0) set(Equal, i0)
Attachment #8813427 -
Flags: review?(lhansen)
Thanks for the suggestions, this code looks nicer! Out of curiosity, what is the origin of the "i" in variable "i0"? (And also "x0" elsewhere)
Attachment #8813427 -
Attachment is obsolete: true
Attachment #8813848 -
Flags: review?(lhansen)
Reporter | ||
Comment 4•7 years ago
|
||
(In reply to David Major [:dmajor] from comment #3) > Created attachment 8813848 [details] [diff] [review] > eqzi64 v2 > > Out of curiosity, what is the origin of the "i" in variable "i0"? (And also > "x0" elsewhere) The "x" for 64-bit comes from Arm64. The "i" is probably a result of my trying to avoid the generic "r" and trying to be mnemonic about what the variable is for. "0" probably just means I expected there to be more registers. Truly, my local variable naming conventions are inconsistent and arbitrary.
Reporter | ||
Updated•7 years ago
|
Attachment #8813848 -
Flags: review?(lhansen) → review+
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → dmajor
Pushed by dmajor@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/30798ceb7d63 Wasm baseline: Avoid using a zero temp register for EqzI64. r=lth
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/30798ceb7d63
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•