Closed Bug 864400 Opened 7 years ago Closed 7 years ago

IonMonkey: Optimize ModI for power-of-two divisor

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch WIP (obsolete) — Splinter Review
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.
Attached patch PatchSplinter Review
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%
Attachment #740359 - Attachment is obsolete: true
Attachment #772064 - Flags: review?(hv1989)
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.
Attachment #772064 - Flags: review?(hv1989) → review+
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.
> 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...
Oops sorry, pressed the wrong tab in the browser... shame on me :-(
https://hg.mozilla.org/mozilla-central/rev/f1088655c731
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 892291
No longer depends on: 892291
You need to log in before you can comment on or make changes to this bug.