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

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

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.

Comment 1

[Daniel Näslund [:dannas]]

Attachment: bug1326160_omit_div_by_zero_and_overflowcheck_for_some_constants.patch

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

Comment 2

[Daniel Näslund [:dannas]]

Attachment: bug1326160_benchmark_results.txt

Comment 3

[Lars T Hansen [:lth]]

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

Comment 4

[Daniel Näslund [:dannas]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0052e450bb38

Comment 5

[Daniel Näslund [:dannas]]

Attachment: bug1326160_v2_omit_div_by_zero_and_overflowcheck_for_some_constants.patch

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/470abd84f545
Omit divide by zero and overflow checks for some constants. r=lth

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/470abd84f545