864400 - IonMonkey: Optimize ModI for power-of-two divisor

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: WIP

Currently Ion lowers MMod to LModPowTwoI if the divisor is a constant power-of-two, but if the divisor is not a constant we use the generic LModI instruction.

The attached WIP patch adds code to LModI codegen to check for a power of two divisor, and do a fast bit-and operation in that case as well. This wins about 50 ms on Kraken audio-oscillator (160 -> 110 ms) and should close most of the gap with V8 on that test.

Bug 837334 is related, but I think we should do both: bug 837334 may make us a bit faster in this case, but this bug should make us more robust when the RHS can have multiple values (and some of them power of two).

Patch passes jit-tests --ion, main TODO is ARM support.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

See comment 0. The ARM implementation is very different so I'd like to file a follow-up bug for investigating a similar optimization there. We do a callWithABI on ARM and it's possible that function is already doing something similar, or maybe it's not as expensive as on x86/x64 and it won't help us much.

This is still about a 50 ms win on audio-oscillator:

oscillator:              1.45x as fast      165.0ms +/- 0.5%    114.1ms +/- 1.0%

Comment 2

[Hannes Verschore [:h4writer]]

Comment on attachment 772064 [details] [diff] [review]
Patch

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

::: js/src/ion/MIR.cpp
@@ +1170,5 @@
>  }
>  
> +bool
> +MMod::maybeDivideByZero() const
> +{

Currently we use 'canBe' instead of 'maybe'. Let us use the same for all.
So I would suggest to use 'canBe' here.

I also find it unnatural to use divideByZere on mod, but that could be because my english isn't good enough. I know it is called divisor... I would grasp to canBeZeroDivisor or something. So I'm fine with using canBeDivideByZero too.

@@ +1177,5 @@
> +}
> +
> +bool
> +MMod::maybeNegativeDividend() const
> +{

canBe

@@ +1184,5 @@
> +}
> +
> +bool
> +MMod::maybePowerOfTwoDivisor() const
> +{

canBe

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +885,5 @@
>      }
>  
>      // Otherwise, we have to beware of two special cases:
> +    if (ins->mir()->maybeNegativeDividend()) {
> +        masm.jump(&done);

I would keep the jump at the same location as before. There is no gain in moving this. Since if a jump is followed directly by the label, we don't emit the jump.

Comment 3

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1088655c731

(In reply to Hannes Verschore [:h4writer] from comment #2)
> Currently we use 'canBe' instead of 'maybe'. Let us use the same for all.

Oops, thanks.

> I would keep the jump at the same location as before. There is no gain in
> moving this. Since if a jump is followed directly by the label, we don't
> emit the jump.

We *do* emit the jump in that case. We don't know where the label is going to end up so it's not easy to optimize.

Comment 4

[Onno]

> https://hg.mozilla.org/integration/mozilla-inbound/rev/f1088655c731

The inbound link shows a different patch than the one of comment 1, it also says that the patch is for bug 890524...

Comment 5

[Onno]

Oops sorry, pressed the wrong tab in the browser... shame on me :-(

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f1088655c731