From 98281c62f0d7650eb7af8cdea61d9833cf9b7841 Mon Sep 17 00:00:00 2001 From: "danno@chromium.org" Date: Wed, 3 Apr 2013 14:45:39 +0000 Subject: [PATCH] Ensure UseRegisterAtStart not used with fixed temp/return register R=vegorov@chromium.org BUG=chromium:201590 Review URL: https://codereview.chromium.org/13527007 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@14124 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/arm/lithium-arm.cc | 35 ++++++++++++++++-- src/ia32/lithium-ia32.cc | 35 +++++++++++++++++- src/x64/lithium-x64.cc | 29 +++++++++++++++ test/mjsunit/regress/regress-201590.js | 66 ++++++++++++++++++++++++++++++++++ 4 files changed, 162 insertions(+), 3 deletions(-) create mode 100644 test/mjsunit/regress/regress-201590.js diff --git a/src/arm/lithium-arm.cc b/src/arm/lithium-arm.cc index dda19bd..7ef1bbe 100644 --- a/src/arm/lithium-arm.cc +++ b/src/arm/lithium-arm.cc @@ -871,6 +871,35 @@ void LChunkBuilder::VisitInstruction(HInstruction* current) { LInstruction* instr = current->CompileToLithium(this); if (instr != NULL) { +#if DEBUG + // Make sure that the lithium instruction has either no fixed register + // constraints in temps or the result OR no uses that are only used at + // start. If this invariant doesn't hold, the register allocator can decide + // to insert a split of a range immediately before the instruction due to an + // already allocated register needing to be used for the instruction's fixed + // register constraint. In this case, The register allocator won't see an + // interference between the split child and the use-at-start (it would if + // the it was just a plain use), so it is free to move the split child into + // the same register that is used for the use-at-start. + // See https://code.google.com/p/chromium/issues/detail?id=201590 + if (!(instr->ClobbersRegisters() && instr->ClobbersDoubleRegisters())) { + int fixed = 0; + int used_at_start = 0; + for (UseIterator it(instr); !it.Done(); it.Advance()) { + LUnallocated* operand = LUnallocated::cast(it.Current()); + if (operand->IsUsedAtStart()) ++used_at_start; + } + if (instr->Output() != NULL) { + if (LUnallocated::cast(instr->Output())->HasFixedPolicy()) ++fixed; + } + for (TempIterator it(instr); !it.Done(); it.Advance()) { + LUnallocated* operand = LUnallocated::cast(it.Current()); + if (operand->HasFixedPolicy()) ++fixed; + } + ASSERT(fixed == 0 || used_at_start == 0); + } +#endif + if (FLAG_stress_pointer_maps && !instr->HasPointerMap()) { instr = AssignPointerMap(instr); } @@ -1115,7 +1144,7 @@ LInstruction* LChunkBuilder::DoUnaryMathOperation(HUnaryMathOperation* instr) { LUnaryMathOperation* result = new(zone()) LUnaryMathOperation(input, temp); return DefineFixedDouble(result, d2); } else { - LOperand* input = UseRegisterAtStart(instr->value()); + LOperand* input = UseRegister(instr->value()); LOperand* temp = (op == kMathRound) ? FixedTemp(d3) : NULL; LUnaryMathOperation* result = new(zone()) LUnaryMathOperation(input, temp); @@ -1823,11 +1852,13 @@ LInstruction* LChunkBuilder::DoChange(HChange* instr) { return AssignEnvironment(DefineAsRegister(res)); } else { ASSERT(to.IsInteger32()); - LOperand* value = UseRegisterAtStart(instr->value()); + LOperand* value = NULL; LInstruction* res = NULL; if (instr->value()->type().IsSmi()) { + value = UseRegisterAtStart(instr->value()); res = DefineAsRegister(new(zone()) LSmiUntag(value, false)); } else { + value = UseRegister(instr->value()); LOperand* temp1 = TempRegister(); LOperand* temp2 = instr->CanTruncateToInt32() ? TempRegister() : NULL; diff --git a/src/ia32/lithium-ia32.cc b/src/ia32/lithium-ia32.cc index f2aec99..0ec5d71 100644 --- a/src/ia32/lithium-ia32.cc +++ b/src/ia32/lithium-ia32.cc @@ -923,6 +923,35 @@ void LChunkBuilder::VisitInstruction(HInstruction* current) { LInstruction* instr = current->CompileToLithium(this); if (instr != NULL) { +#if DEBUG + // Make sure that the lithium instruction has either no fixed register + // constraints in temps or the result OR no uses that are only used at + // start. If this invariant doesn't hold, the register allocator can decide + // to insert a split of a range immediately before the instruction due to an + // already allocated register needing to be used for the instruction's fixed + // register constraint. In this case, The register allocator won't see an + // interference between the split child and the use-at-start (it would if + // the it was just a plain use), so it is free to move the split child into + // the same register that is used for the use-at-start. + // See https://code.google.com/p/chromium/issues/detail?id=201590 + if (!(instr->ClobbersRegisters() && instr->ClobbersDoubleRegisters())) { + int fixed = 0; + int used_at_start = 0; + for (UseIterator it(instr); !it.Done(); it.Advance()) { + LUnallocated* operand = LUnallocated::cast(it.Current()); + if (operand->IsUsedAtStart()) ++used_at_start; + } + if (instr->Output() != NULL) { + if (LUnallocated::cast(instr->Output())->HasFixedPolicy()) ++fixed; + } + for (TempIterator it(instr); !it.Done(); it.Advance()) { + LUnallocated* operand = LUnallocated::cast(it.Current()); + if (operand->HasFixedPolicy()) ++fixed; + } + ASSERT(fixed == 0 || used_at_start == 0); + } +#endif + if (FLAG_stress_pointer_maps && !instr->HasPointerMap()) { instr = AssignPointerMap(instr); } @@ -1182,16 +1211,20 @@ LInstruction* LChunkBuilder::DoUnaryMathOperation(HUnaryMathOperation* instr) { input); return MarkAsCall(DefineFixedDouble(result, xmm1), instr); } else { - LOperand* input = UseRegisterAtStart(instr->value()); LOperand* context = UseAny(instr->context()); // Deferred use by MathAbs. + LOperand* input = NULL; if (op == kMathPowHalf) { + input = UseRegisterAtStart(instr->value()); LOperand* temp = TempRegister(); LMathPowHalf* result = new(zone()) LMathPowHalf(context, input, temp); return DefineSameAsFirst(result); } else if (op == kMathRound) { + input = UseRegister(instr->value()); LOperand* temp = FixedTemp(xmm4); LMathRound* result = new(zone()) LMathRound(context, input, temp); return AssignEnvironment(DefineAsRegister(result)); + } else { + input = UseRegisterAtStart(instr->value()); } LUnaryMathOperation* result = new(zone()) LUnaryMathOperation(context, input); diff --git a/src/x64/lithium-x64.cc b/src/x64/lithium-x64.cc index 16248ee..5f62400 100644 --- a/src/x64/lithium-x64.cc +++ b/src/x64/lithium-x64.cc @@ -879,6 +879,35 @@ void LChunkBuilder::VisitInstruction(HInstruction* current) { LInstruction* instr = current->CompileToLithium(this); if (instr != NULL) { +#if DEBUG + // Make sure that the lithium instruction has either no fixed register + // constraints in temps or the result OR no uses that are only used at + // start. If this invariant doesn't hold, the register allocator can decide + // to insert a split of a range immediately before the instruction due to an + // already allocated register needing to be used for the instruction's fixed + // register constraint. In this case, The register allocator won't see an + // interference between the split child and the use-at-start (it would if + // the it was just a plain use), so it is free to move the split child into + // the same register that is used for the use-at-start. + // See https://code.google.com/p/chromium/issues/detail?id=201590 + if (!(instr->ClobbersRegisters() && instr->ClobbersDoubleRegisters())) { + int fixed = 0; + int used_at_start = 0; + for (UseIterator it(instr); !it.Done(); it.Advance()) { + LUnallocated* operand = LUnallocated::cast(it.Current()); + if (operand->IsUsedAtStart()) ++used_at_start; + } + if (instr->Output() != NULL) { + if (LUnallocated::cast(instr->Output())->HasFixedPolicy()) ++fixed; + } + for (TempIterator it(instr); !it.Done(); it.Advance()) { + LUnallocated* operand = LUnallocated::cast(it.Current()); + if (operand->HasFixedPolicy()) ++fixed; + } + ASSERT(fixed == 0 || used_at_start == 0); + } +#endif + if (FLAG_stress_pointer_maps && !instr->HasPointerMap()) { instr = AssignPointerMap(instr); } diff --git a/test/mjsunit/regress/regress-201590.js b/test/mjsunit/regress/regress-201590.js new file mode 100644 index 0000000..0e7ba57 --- /dev/null +++ b/test/mjsunit/regress/regress-201590.js @@ -0,0 +1,66 @@ +// Copyright 2013 the V8 project authors. All rights reserved. +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following +// disclaimer in the documentation and/or other materials provided +// with the distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived +// from this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// Flags: --allow-natives-syntax + +var gdpRatio = 16/9; + +function Foo(initialX, initialY, initialScale, initialMapHeight) { + this.ORIGIN = { x: initialX, y: initialY }; + this.scale = initialScale; + this.mapHeight = initialMapHeight; +} + +Foo.prototype.bar = function (x, y, xOffset, yOffset) { + var tileHeight = 64 * 0.25 * this.scale, + tileWidth = 128 * 0.25 * this.scale, + xOffset = xOffset * 0.5 || 0, + yOffset = yOffset * 0.5 || 0; + var xPos = ((xOffset) * gdpRatio) + this.ORIGIN.x * this.scale - + ((y * tileWidth ) * gdpRatio) + ((x * tileWidth ) * gdpRatio); + var yPos = ((yOffset) * gdpRatio) + this.ORIGIN.y * this.scale + + ((y * tileHeight) * gdpRatio) + ((x * tileHeight) * gdpRatio); + xPos = xPos - Math.round(((tileWidth) * gdpRatio)) + + this.mapHeight * Math.round(((tileWidth) * gdpRatio)); + return { + x: Math.floor(xPos), + y: Math.floor(yPos) + }; +} + +var f = new Foo(10, 20, 2.5, 400); + +function baz() { + var b = f.bar(1.1, 2.2, 3.3, 4.4); + assertEquals(56529, b.x); + assertEquals(288, b.y); +} + +baz(); +baz(); +%OptimizeFunctionOnNextCall(Foo.prototype.bar); +baz(); -- 2.7.4