Closed Bug 806643 Opened 12 years ago Closed 12 years ago

IonMonkey: Negate Doubles by Flipping Signed Bit

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: sstangl, Unassigned)

References

Details

(Whiteboard: [ion:t])

Attachments

(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 v2 (deleted) — Splinter 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; } 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!
Attachment #676522 - Flags: review+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: