Closed Bug 1316848 Opened 5 years ago Closed 5 years ago

Wasm baseline: Avoid using a zero temp register for EqzI64


(Core :: JavaScript Engine: JIT, defect, P3)




Tracking Status
firefox53 --- fixed


(Reporter: lth, Assigned: away)


(Blocks 1 open bug)



(1 file, 1 obsolete file)

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.
Attached patch eqzi64 (obsolete) — Splinter Review
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)
Comment on attachment 8813427 [details] [diff] [review]

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)
Attached patch eqzi64 v2Splinter Review
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)
(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+
Assignee: nobody → dmajor
Pushed by
Wasm baseline: Avoid using a zero temp register for EqzI64. r=lth
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.