Wasm baseline: Division by nonzero constants can omit divide-by-zero check

RESOLVED FIXED in Firefox 53

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: lth, Assigned: dannas)

Tracking

(Blocks 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Bug 1316803 handles the case where the divisor is a positive constant power of two, but if the divisor is any other nonzero constant on the slow path we can at least omit the divide-by-zero check.  There may also be constants for which we can omit the signed overflow check, I'm not sure yet.

The code would look like this, probably:

    int32_t c;
    bool isConst = peekConstI32();
    RegI32 r0, r1;
    pop2xI32ForIntMulDiv(&r0, &r1);

    Label done;
    if (!isConst || c == 0)
        checkDivideByZeroI32(r1, r0, &done);
    if (!isConst || c requires a check)
        checkDivideSignedOverflowI32(r1, r0, &done, ZeroOnOverflow(false));

Really checkDivideSignedOverflowI32() performs two comparisons, one on r0 and one on r1, and the one on r1 can be omitted if c != -1, so there is a little more complexity here.

Not sure how much difference this will make since division is relatively uncommon and the main operation is pretty slow anyway, but it's reasonable hygiene, cf bug 1316824.
Added checks for quotient and remainder.

AFAIU, division overflow can only happen when we divide INT_MIN with -1. All other expressions will fit in the size of the type. So I only check against rhs == -1. I wonder if I'm missing something obvious here, this feels a little too straight forward.

>Really checkDivideSignedOverflowI32() performs two comparisons, one on r0 and one on r1, and the one on r1 can be omitted if c != -1, so there is a little more complexity here.

But we only need to determine if rhs is -1. Only then do we need to call checkDivideSignedOverflowI32.

>Not sure how much difference this will make since division is relatively uncommon and the main operation is pretty slow anyway, but it's reasonable hygiene, cf bug 1316824.

The benchmarks numbers are mostly stable but I saw some speedups for the conditionals and copy test when running embenchen. Will attach the benchmark results
Assignee: nobody → dannas
Status: NEW → ASSIGNED
Attachment #8826330 - Flags: review?(lhansen)
Comment on attachment 8826330 [details] [diff] [review]
bug1326160_omit_div_by_zero_and_overflowcheck_for_some_constants.patch

Review of attachment 8826330 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +1716,5 @@
>          stk_.popBack();
>          return true;
>      }
>  
> +    MOZ_MUST_USE bool peekConstI32(int32_t& c) {

People keep beating up on me because the house style apparently is to pass out parameters by pointer (ie, "int32_t* c"), so let's do that here even if that is inconsistent with most existing functions (which we will fix later).

@@ +1724,5 @@
> +        c = v.i32val();
> +        return true;
> +    }
> +
> +    MOZ_MUST_USE bool peekConstI64(int64_t& c) {

And here.

@@ +4342,5 @@
>          Label done;
> +        if (!isConst || c == 0)
> +            checkDivideByZeroI32(r1, r0, &done);
> +        if (!isConst || c == -1) // TODO(dannas): Is this correct?
> +            checkDivideSignedOverflowI32(r1, r0, &done, ZeroOnOverflow(false));

Yes, this does look right.  Only INT_MIN and -1 get special handling, and even though the handling differs by other parameters we can ignore that if we know the rhs is not -1.  (Don't forget to remove the TODO.)
Attachment #8826330 - Flags: review?(lhansen) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/470abd84f545
Omit divide by zero and overflow checks for some constants. r=lth
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/470abd84f545
Status: ASSIGNED → 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.