Closed
Bug 806643
Opened 12 years ago
Closed 12 years ago
IonMonkey: Negate Doubles by Flipping Signed Bit
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: sstangl, Unassigned)
References
Details
(Whiteboard: [ion:t])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Fyi, on arm the MNF instruction is VNEG (this is also what it is called in the IonMonkey assembler.)
Comment 2•12 years ago
|
||
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•12 years ago
|
||
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
Reporter | ||
Comment 4•12 years ago
|
||
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+
Reporter | ||
Comment 5•12 years ago
|
||
Comment 6•12 years ago
|
||
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.
Description
•