Closed Bug 1316848 Opened 4 years ago Closed 4 years ago
Wasm baseline: Avoid using a zero temp register for Eqz
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".
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)
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)
(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.
Attachment #8813848 - Flags: review?(lhansen) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/30798ceb7d63 Wasm baseline: Avoid using a zero temp register for EqzI64. r=lth
You need to log in before you can comment on or make changes to this bug.