Wasm baseline: Avoid using a zero temp register for EqzI64

RESOLVED FIXED in Firefox 53

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: lth, Assigned: dmajor)

Tracking

(Blocks 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

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

Comment 1

3 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)
Reporter

Comment 2

3 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)
Assignee

Comment 3

3 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)
Reporter

Comment 4

3 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

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

Updated

3 years ago
Assignee: nobody → dmajor

Comment 5

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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/30798ceb7d63
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.