995934 - IonMonkey: math.round has extra code

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

The first part of codegenerator of MRound looks like (on x86 only):

> masm.loadConstantDouble(0.5, temp);       
> masm.xorpd(scratch, scratch);
> masm.branchDouble(Assembler::DoubleLessThan, input, scratch, &negative);
> Assembler::Condition bailCond = masm.testNegativeZero(input, output);
> bailoutIf(bailCond, lir->snapshot());

Which translates in:

> 0xb720ef41:  movsd  0xb720f0c0,%xmm1
> 0xb720ef49:  xorpd  %xmm7,%xmm7
> 0xb720ef4d:  ucomisd %xmm0,%xmm7
> 0xb720ef51:  ja     0xb720ef8b
> 0xb720ef57:  xorpd  %xmm7,%xmm7
> 0xb720ef5b:  ucomisd %xmm7,%xmm0
> 0xb720ef5f:  jne    0xb720ef6c

So we do the "same" comparison twice and could have written:

> 0xb720ef41:  movsd  0xb720f0c0,%xmm1
> 0xb720ef49:  xorpd  %xmm7,%xmm7
> 0xb720ef4d:  ucomisd %xmm0,%xmm7
> 0xb720ef51:  ja     0xb720ef8b
> 0xb720ef5f:  jne    0xb720ef6c

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Part 1: Remove double compare

This fixes the issue of the two compares in testNegativeZero and simplifies logic in there.

1) Rename testNegativeZeroXXX to branchNegativeZeroXXX.

Doesn't do the stupid part of:

>   branch if not 0 to nonzero
>   ...
>   andl something to set flags
> nonzero:
>   branch to bailout path if some flags are set

but

>   branch if not 0 to nonzero
>   ...
>   branch if zero to failed label
> nonzero:

So we moved the second branch out of the hot path.

2) Move branchNegativeZeroXXX to shared x86 assembler.

- branchNegativeZeroFloat32 has same code for x64 and x86. So no issue.
- We want to use this function for convertDoubleToInt32 which is in shared x86 assembler.
=> Note: I could also duplicate convertDoubleToInt32 in both architectures. Arguable, but I think it doesn't matter at all.
- Fix bug 770630 by doing this

On small testcase:

function test() {
  var a = 0
  for (var i=0; i<100000000; i++) {
    a = Math.round(i+0.1);
  }
  return a
}

var start = new Date();
test()
print(new Date() - start)

I see time go from 220ms to 190ms.

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Part 2: Jump to positives instead of running

This was part2. Where I would add branchZeroIsNegativeZero, which would only test if a 0 is a -0.0 or +0.0. Looking at performance we are not blocked by the computation but by the jump. So calculating it twice doesn't matter. Leaving it for now.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8410234 [details] [diff] [review]
Part 1: Remove double compare

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

Thanks for doing that!

::: js/src/jit/shared/MacroAssembler-x86-shared.cpp
@@ +159,5 @@
> +MacroAssemblerX86Shared::branchNegativeZero(const FloatRegister &reg,
> +                                            const Register &scratch,
> +                                            Label *label)
> +{
> +    // Determines whether the single double contained in the XMM register reg

s/the single double/the low double

@@ +160,5 @@
> +                                            const Register &scratch,
> +                                            Label *label)
> +{
> +    // Determines whether the single double contained in the XMM register reg
> +    // is equal to double-precision -0.

I'd remove "double-precision" here as -0 implicitly refers to a double, but up to you.

@@ +176,5 @@
> +    movmskpd(reg, scratch);
> +
> +    // If reg is 1 or 3, input is negative zero.
> +    // If reg is 0 or 2, input is a normal zero.
> +    branchTest32(NonZero, scratch, Imm32(1), label);

Nice! It makes much more sense.

@@ +179,5 @@
> +    // If reg is 0 or 2, input is a normal zero.
> +    branchTest32(NonZero, scratch, Imm32(1), label);
> +
> +    bind(&nonZero);
> +#elif defined(JS_CODEGEN_X64)

It sort of doesn't make sense to have these #ifdef here, unless you have in mind the future work consisting in merging MacroAssemblers into a single one. I'd keep these in the arch specific MacroAssemblers in the meanwhile.

Comment 4

[Hannes Verschore [:h4writer]]

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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