Closed Bug 806643 Opened 7 years ago Closed 7 years ago

IonMonkey: Negate Doubles by Flipping Signed Bit


(Core :: JavaScript Engine, defect)

Not set





(Reporter: sstangl, Unassigned)



(Whiteboard: [ion:t])


(1 file, 1 obsolete file)

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.
Fyi, on arm the MNF instruction is VNEG (this is also what it is called in the IonMonkey assembler.)
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.
Attached patch Patch v2Splinter Review
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.
Attachment #676507 - Attachment is obsolete: true
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;

::: 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);

Attachment #676522 - Flags: review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.