806643 - IonMonkey: Negate Doubles by Flipping Signed Bit

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

Given a double D, (-D) is currently emitted and lowered as:

> MDouble // D
> MConstant(-1.0000)
> MMul(MConstant, MDouble)

which does several unnecessary things:

1) loads D into a double register, perhaps unnecessarily
2) emits and loads a double constant: -1.0 does not get optimized by maybeInlineDouble()
3) performs an unnecessary multiplication

Instead, we should lower to some MNegD, which takes a stack address or a double register:

- If a stack address, negate the 64th bit using a GPR.
- If a double register:
- - On ARM, use MNF to negate.
- - on x86 and x86_64, use maybeInlineDouble(-0, ScratchFloatReg), then xorpd().

Case occurs in v8/raytrace.js, Flog.RayTracer.Shape.Sphere.prototype.intersect(), on line 438.
Most likely no noticeable performance impact due to relative size of the benchmark -- just noting it.

Potentially a good first bug.

Comment 1

[Marty Rosenberg [:mjrosenb]]

Fyi, on arm the MNF instruction is VNEG (this is also what it is called in the IonMonkey assembler.)

Comment 2

[Tony Young]

Attachment: Partial fix -- negates all values in a float register

This patch addresses part of the issue -- the value is XOR'd with -0.0 in a scratch float register for negations rather than load + mul. Unsure of how to negate first 32-bits when value is on the stack.

Comment 3

[Tony Young]

Attachment: Patch v2

This patch revises CodeGeneratorX86Shared::visitNegD to use the instructions from MacroAssemblerX86Shared::maybeInlineDouble to generate -1. Also, since IonMonkey has no way to negate a value on the stack, this patch no longer seeks to address negating the double while keeping it on the stack.

Comment 4

[Sean Stangl [:sstangl]]

Comment on attachment 676522 [details] [diff] [review]
Patch v2

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

Great patch, everything looks good! On a small microbenchmark (below), this patch makes double negation about 3x as fast as it is currently.

function f() {
  var x = 1.5;
  for (var i = 0; i < 100000000; i++) {
    x = -x;
    x = -x;
    x = -x;
    x = -x;
    x = -x;
  }
  return x;
}
f();

::: js/src/ion/Lowering.cpp
@@ +856,5 @@
>      if (ins->specialization() == MIRType_Double) {
>          JS_ASSERT(lhs->type() == MIRType_Double);
> +
> +        // If our LHS is a constant -1.0, we can optimize to an LNegD.
> +        if (lhs->op() == MDefinition::Op_Constant &&

nit: if (lhs->isConstant() && lhs->toConstant()->value() == DoubleValue(-1.0)) {

SpiderMonkey C++ style also prefers putting { on its own line if the conditional expression spans multiple lines.

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +454,5 @@
> +    JS_ASSERT(input == ToFloatRegister(ins->output()));
> +
> +    // From MacroAssemblerX86Shared::maybeInlineDouble
> +    masm.pcmpeqw(ScratchFloatReg, ScratchFloatReg);
> +    masm.psllq(Imm32(63), ScratchFloatReg);

Great!

Comment 5

[Sean Stangl [:sstangl]]

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

Comment 6

[Ed Morley [:emorley]]

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