Wasm baseline: Avoid using a zero temp register for EqzI64

RESOLVED FIXED in Firefox 53



3 years ago
2 years ago


(Reporter: lth, Assigned: dmajor)


(Blocks 1 bug)


Firefox Tracking Flags

(firefox53 fixed)



(1 attachment, 1 obsolete attachment)



3 years ago
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.

Comment 1

2 years ago
Posted 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 2

2 years ago
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)

Comment 3

2 years ago
Posted 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)

Comment 4

2 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.


2 years ago
Attachment #8813848 - Flags: review?(lhansen) → review+


2 years ago
Assignee: nobody → dmajor

Comment 5

2 years ago
Pushed by dmajor@mozilla.com:
Wasm baseline: Avoid using a zero temp register for EqzI64. r=lth

Comment 6

2 years ago
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.