Revision: 7101
Author: [email protected]
Date: Wed Mar 9 05:10:51 2011
Log: Merge revisions 6871, 6981, and 7082 to the 3.0 branch.
These changes fix three different stack-height mismatch issues with
deoptimization.
Review URL: http://codereview.chromium.org/6651019
http://code.google.com/p/v8/source/detail?r=7101
Added:
/branches/3.0/test/mjsunit/regress/regress-1210.js
/branches/3.0/test/mjsunit/regress/regress-1237.js
Modified:
/branches/3.0/src/arm/full-codegen-arm.cc
/branches/3.0/src/hydrogen.cc
/branches/3.0/src/ia32/full-codegen-ia32.cc
/branches/3.0/src/version.cc
/branches/3.0/src/x64/full-codegen-x64.cc
/branches/3.0/test/mjsunit/regress/regress-1167.js
=======================================
--- /dev/null
+++ /branches/3.0/test/mjsunit/regress/regress-1210.js Wed Mar 9 05:10:51
2011
@@ -0,0 +1,48 @@
+// Copyright 2011 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.
+
+// Deoptimization of the key expression in an arguments access should see
+// the arguments object as the value of the receiver.
+
+var a = 0;
+
+function observe(x, y) { return x; }
+
+function side_effect(x) { a = x; }
+
+function test() {
+ // We will trigger deoptimization of 'a + 0' which should bail out to
+ // immediately after the call to 'side_effect' (i.e., still in the key
+ // subexpression of the arguments access).
+ return observe(a, arguments[side_effect(a), a + 0]);
+}
+
+// Run enough to optimize assuming global 'a' is a smi.
+for (var i = 0; i < 1000000; ++i) test(0);
+
+a = "hello";
+test(0);
=======================================
--- /dev/null
+++ /branches/3.0/test/mjsunit/regress/regress-1237.js Wed Mar 9 05:10:51
2011
@@ -0,0 +1,36 @@
+// Copyright 2011 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.
+
+// Deoptimization after a conditional expression in an effect context
should
+// not see the value of the expression.
+function observe(x, y) { return x; }
+function test(x) {
+ return observe(1, ((x? observe(observe.prototype.x): 'c'), x + 1));
+}
+
+for (var i = 0; i < 10000000; ++i) test(0);
+test("a");
=======================================
--- /branches/3.0/src/arm/full-codegen-arm.cc Tue Mar 1 03:42:42 2011
+++ /branches/3.0/src/arm/full-codegen-arm.cc Wed Mar 9 05:10:51 2011
@@ -3010,17 +3010,23 @@
case Token::NOT: {
Comment cmnt(masm_, "[ UnaryOperation (NOT)");
- Label materialize_true, materialize_false;
- Label* if_true = NULL;
- Label* if_false = NULL;
- Label* fall_through = NULL;
-
- // Notice that the labels are swapped.
- context()->PrepareTest(&materialize_true, &materialize_false,
- &if_false, &if_true, &fall_through);
- if (context()->IsTest()) ForwardBailoutToChild(expr);
- VisitForControl(expr->expression(), if_true, if_false, fall_through);
- context()->Plug(if_false, if_true); // Labels swapped.
+ if (context()->IsEffect()) {
+ // Unary NOT has no side effects so it's only necessary to visit
the
+ // subexpression. Match the optimizing compiler by not branching.
+ VisitForEffect(expr->expression());
+ } else {
+ Label materialize_true, materialize_false;
+ Label* if_true = NULL;
+ Label* if_false = NULL;
+ Label* fall_through = NULL;
+
+ // Notice that the labels are swapped.
+ context()->PrepareTest(&materialize_true, &materialize_false,
+ &if_false, &if_true, &fall_through);
+ if (context()->IsTest()) ForwardBailoutToChild(expr);
+ VisitForControl(expr->expression(), if_true, if_false,
fall_through);
+ context()->Plug(if_false, if_true); // Labels swapped.
+ }
break;
}
=======================================
--- /branches/3.0/src/hydrogen.cc Thu Feb 17 05:41:56 2011
+++ /branches/3.0/src/hydrogen.cc Wed Mar 9 05:10:51 2011
@@ -307,6 +307,13 @@
ASSERT(a == b);
}
}
+
+ // Check that the incoming edges are in edge split form.
+ if (predecessors_.length() > 1) {
+ for (int i = 0; i < predecessors_.length(); ++i) {
+ ASSERT(predecessors_[i]->end()->SecondSuccessor() == NULL);
+ }
+ }
}
#endif
@@ -2900,14 +2907,23 @@
then_graph->entry_block(),
else_graph->entry_block());
+ // Visit the true and false subexpressions in the same AST context as the
+ // whole expression.
then_graph->entry_block()->SetJoinId(expr->ThenId());
- ADD_TO_SUBGRAPH(then_graph, expr->then_expression());
+ { SubgraphScope scope(this, then_graph);
+ Visit(expr->then_expression());
+ CHECK_BAILOUT;
+ }
else_graph->entry_block()->SetJoinId(expr->ElseId());
- ADD_TO_SUBGRAPH(else_graph, expr->else_expression());
-
- current_subgraph_->AppendJoin(then_graph, else_graph, expr);
- ast_context()->ReturnValue(Pop());
+ { SubgraphScope scope(this, else_graph);
+ Visit(expr->else_expression());
+ CHECK_BAILOUT;
+ }
+
+ if (!ast_context()->IsTest()) {
+ subgraph()->AppendJoin(then_graph, else_graph, expr);
+ }
}
@@ -3742,9 +3758,11 @@
HInstruction* elements = AddInstruction(new HArgumentsElements);
result = new HArgumentsLength(elements);
} else {
+ Push(graph()->GetArgumentsObject());
VisitForValue(expr->key());
if (HasStackOverflow()) return false;
HValue* key = Pop();
+ Drop(1); // Arguments object.
HInstruction* elements = AddInstruction(new HArgumentsElements);
HInstruction* length = AddInstruction(new HArgumentsLength(elements));
AddInstruction(new HBoundsCheck(key, length));
=======================================
--- /branches/3.0/src/ia32/full-codegen-ia32.cc Tue Mar 1 03:42:42 2011
+++ /branches/3.0/src/ia32/full-codegen-ia32.cc Wed Mar 9 05:10:51 2011
@@ -3504,17 +3504,22 @@
case Token::NOT: {
Comment cmnt(masm_, "[ UnaryOperation (NOT)");
-
- Label materialize_true, materialize_false;
- Label* if_true = NULL;
- Label* if_false = NULL;
- Label* fall_through = NULL;
- // Notice that the labels are swapped.
- context()->PrepareTest(&materialize_true, &materialize_false,
- &if_false, &if_true, &fall_through);
- if (context()->IsTest()) ForwardBailoutToChild(expr);
- VisitForControl(expr->expression(), if_true, if_false, fall_through);
- context()->Plug(if_false, if_true); // Labels swapped.
+ if (context()->IsEffect()) {
+ // Unary NOT has no side effects so it's only necessary to visit
the
+ // subexpression. Match the optimizing compiler by not branching.
+ VisitForEffect(expr->expression());
+ } else {
+ Label materialize_true, materialize_false;
+ Label* if_true = NULL;
+ Label* if_false = NULL;
+ Label* fall_through = NULL;
+ // Notice that the labels are swapped.
+ context()->PrepareTest(&materialize_true, &materialize_false,
+ &if_false, &if_true, &fall_through);
+ if (context()->IsTest()) ForwardBailoutToChild(expr);
+ VisitForControl(expr->expression(), if_true, if_false,
fall_through);
+ context()->Plug(if_false, if_true); // Labels swapped.
+ }
break;
}
=======================================
--- /branches/3.0/src/version.cc Wed Mar 2 04:02:18 2011
+++ /branches/3.0/src/version.cc Wed Mar 9 05:10:51 2011
@@ -35,7 +35,7 @@
#define MAJOR_VERSION 3
#define MINOR_VERSION 0
#define BUILD_NUMBER 12
-#define PATCH_LEVEL 30
+#define PATCH_LEVEL 31
#define CANDIDATE_VERSION false
// Define SONAME to have the SCons build the put a specific SONAME into the
=======================================
--- /branches/3.0/src/x64/full-codegen-x64.cc Tue Mar 1 03:42:42 2011
+++ /branches/3.0/src/x64/full-codegen-x64.cc Wed Mar 9 05:10:51 2011
@@ -3024,16 +3024,22 @@
case Token::NOT: {
Comment cmnt(masm_, "[ UnaryOperation (NOT)");
- Label materialize_true, materialize_false;
- Label* if_true = NULL;
- Label* if_false = NULL;
- Label* fall_through = NULL;
- // Notice that the labels are swapped.
- context()->PrepareTest(&materialize_true, &materialize_false,
- &if_false, &if_true, &fall_through);
- if (context()->IsTest()) ForwardBailoutToChild(expr);
- VisitForControl(expr->expression(), if_true, if_false, fall_through);
- context()->Plug(if_false, if_true); // Labels swapped.
+ if (context()->IsEffect()) {
+ // Unary NOT has no side effects so it's only necessary to visit
the
+ // subexpression. Match the optimizing compiler by not branching.
+ VisitForEffect(expr->expression());
+ } else {
+ Label materialize_true, materialize_false;
+ Label* if_true = NULL;
+ Label* if_false = NULL;
+ Label* fall_through = NULL;
+ // Notice that the labels are swapped.
+ context()->PrepareTest(&materialize_true, &materialize_false,
+ &if_false, &if_true, &fall_through);
+ if (context()->IsTest()) ForwardBailoutToChild(expr);
+ VisitForControl(expr->expression(), if_true, if_false,
fall_through);
+ context()->Plug(if_false, if_true); // Labels swapped.
+ }
break;
}
=======================================
--- /branches/3.0/test/mjsunit/regress/regress-1167.js Thu Feb 17 05:41:56
2011
+++ /branches/3.0/test/mjsunit/regress/regress-1167.js Wed Mar 9 05:10:51
2011
@@ -27,7 +27,7 @@
// Deoptimization after a logical not in an effect context should not see a
// value for the logical not expression.
-function test(n) {
+function test0(n) {
var a = new Array(n);
for (var i = 0; i < n; ++i) {
// ~ of a non-numeric value is used to trigger deoptimization.
@@ -38,6 +38,35 @@
// OSR (after deoptimization) is used to observe the stack height mismatch.
for (var i = 0; i < 5; ++i) {
for (var j = 1; j < 12; ++j) {
- test(j * 1000);
+ test0(j * 1000);
}
}
+
+
+// Similar test with a different subexpression of unary !.
+function test1(n) {
+ var a = new Array(n);
+ for (var i = 0; i < n; ++i) {
+ a[i] = void(!(- 'object')) % ~(delete 4);
+ }
+}
+
+for (i = 0; i < 5; ++i) {
+ for (j = 1; j < 12; ++j) {
+ test1(j * 1000);
+ }
+}
+
+
+// A similar issue, different subexpression of unary ! (e0 !== e1 is
+// translated into !(e0 == e1)) and different effect context.
+function side_effect() { }
+function observe(x, y) { return x; }
+function test2(x) {
+ return observe(this,
+ (((side_effect.observe <= side_effect.side_effect) !==
false),
+ x + 1));
+}
+
+for (var i = 0; i < 1000000; ++i) test2(0);
+test2(test2);
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
