Author: [email protected]
Date: Mon Mar 9 07:12:20 2009
New Revision: 1453
Added:
branches/bleeding_edge/test/mjsunit/regress/regress-260.js
Modified:
branches/bleeding_edge/src/codegen-ia32.cc
Log:
Work around issue 260 for now by disabling duplication of the loop
condition for while and for loops.
http://code.google.com/p/v8/issues/detail?id=260
Review URL: http://codereview.chromium.org/40294
Modified: branches/bleeding_edge/src/codegen-ia32.cc
==============================================================================
--- branches/bleeding_edge/src/codegen-ia32.cc (original)
+++ branches/bleeding_edge/src/codegen-ia32.cc Mon Mar 9 07:12:20 2009
@@ -2172,15 +2172,16 @@
JumpTarget body(this, JumpTarget::BIDIRECTIONAL);
IncrementLoopNesting();
- // Label the top of the loop for the backward CFG edge. If the test
- // is always true we can use the continue target, and if the test is
- // always false there is no need.
+ // Label the top of the loop for the backward jump if necessary.
if (info == ALWAYS_TRUE) {
+ // Use the continue target.
node->continue_target()->Initialize(this,
JumpTarget::BIDIRECTIONAL);
node->continue_target()->Bind();
} else if (info == ALWAYS_FALSE) {
+ // No need to label it.
node->continue_target()->Initialize(this);
} else {
+ // Continue is the test, so use the backward body target.
ASSERT(info == DONT_KNOW);
node->continue_target()->Initialize(this);
body.Bind();
@@ -2191,8 +2192,8 @@
// Compile the test.
if (info == ALWAYS_TRUE) {
- // If control flow can fall off the end of the body, jump back to
- // the top and bind the break target as the exit.
+ // If control flow can fall off the end of the body, jump back
+ // to the top and bind the break target at the exit.
if (has_valid_frame()) {
node->continue_target()->Jump();
}
@@ -2228,15 +2229,19 @@
}
case LoopStatement::WHILE_LOOP: {
- JumpTarget body(this, JumpTarget::BIDIRECTIONAL);
+ // TODO(260): This flag controls whether to duplicate the test
+ // at the bottom of the loop. Replace it with a better
+ // indication of when it is safe to do so.
+ static const bool test_at_bottom = false;
+
+ JumpTarget body; // Uninitialized.
IncrementLoopNesting();
// If the condition is always false and has no side effects, we
// do not need to compile anything.
if (info == ALWAYS_FALSE) break;
- // Based on the condition analysis, compile the test if
- // necessary and label the body if necessary.
+ // Based on the condition analysis, compile the test as necessary.
if (info == ALWAYS_TRUE) {
// We will not compile the test expression. Label the top of
// the loop with the continue target.
@@ -2244,7 +2249,18 @@
node->continue_target()->Bind();
} else {
ASSERT(info == DONT_KNOW); // ALWAYS_FALSE cannot reach here.
- node->continue_target()->Initialize(this);
+ if (test_at_bottom) {
+ // Continue is the test at the bottom, no need to label the
+ // test at the top. The body is a backward target.
+ node->continue_target()->Initialize(this);
+ body.Initialize(this, JumpTarget::BIDIRECTIONAL);
+ } else {
+ // Label the test at the top as the continue target. The
+ // body is a forward-only target.
+ node->continue_target()->Initialize(this,
JumpTarget::BIDIRECTIONAL);
+ node->continue_target()->Bind();
+ body.Initialize(this);
+ }
// Compile the test with the body as the true target and
// preferred fall-through and with the break target as the
// false target.
@@ -2252,9 +2268,9 @@
LoadCondition(node->cond(), NOT_INSIDE_TYPEOF, &dest, true);
if (dest.false_was_fall_through()) {
- // If we don't have dangling jumps to the body, the test is
- // unconditionally false and we do not need to compile the
- // body.
+ // If we got the break target as fall-through, the test may
+ // have been unconditionally false (if there are no jumps to
+ // the body).
if (!body.is_linked()) break;
// Otherwise, jump around the body on the fall through and
@@ -2265,30 +2281,36 @@
}
}
- // The (stack check at the start of the) body was labeled.
- // Compile it.
CheckStack(); // TODO(1222600): ignore if body contains calls.
Visit(node->body());
- // Compile the test if necessary and jump back.
+ // Based on the condition analysis, compile the backward jump as
+ // necessary.
if (info == ALWAYS_TRUE) {
- // The body has been labeled with the continue target.
+ // The loop body has been labeled with the continue target.
if (has_valid_frame()) {
node->continue_target()->Jump();
}
} else {
ASSERT(info == DONT_KNOW); // ALWAYS_FALSE cannot reach here.
- if (node->continue_target()->is_linked()) {
- node->continue_target()->Bind();
- }
-
- // If control can reach the bottom by falling off the body or
- // a continue in the body, (re)compile the test at the bottom.
- if (has_valid_frame()) {
- // The break target is the fall-through (body is a backward
- // jump from here).
- ControlDestination dest(&body, node->break_target(), false);
- LoadCondition(node->cond(), NOT_INSIDE_TYPEOF, &dest, true);
+ if (test_at_bottom) {
+ // If we have chosen to recompile the test at the bottom,
+ // then it is the continue target.
+ if (node->continue_target()->is_linked()) {
+ node->continue_target()->Bind();
+ }
+ if (has_valid_frame()) {
+ // The break target is the fall-through (body is a backward
+ // jump from here and thus an invalid fall-through).
+ ControlDestination dest(&body, node->break_target(), false);
+ LoadCondition(node->cond(), NOT_INSIDE_TYPEOF, &dest, true);
+ }
+ } else {
+ // If we have chosen not to recompile the test at the
+ // bottom, jump back to the one at the top.
+ if (has_valid_frame()) {
+ node->continue_target()->Jump();
+ }
}
}
@@ -2301,7 +2323,13 @@
}
case LoopStatement::FOR_LOOP: {
- JumpTarget body(this, JumpTarget::BIDIRECTIONAL);
+ // TODO(260): This flag controls whether to duplicate the test
+ // at the bottom of the loop. Replace it with a better
+ // indication of when it is safe to do so.
+ static const bool test_at_bottom = false;
+
+ JumpTarget loop(this, JumpTarget::BIDIRECTIONAL);
+ JumpTarget body(this);
// Compile the init expression if present.
if (node->init() != NULL) {
@@ -2314,22 +2342,37 @@
// do not need to compile anything else.
if (info == ALWAYS_FALSE) break;
- // Based on the condition analysis, compile the test if
- // necessary and label the body if necessary.
+ // Based on the condition analysis, compile the test as necessary.
if (info == ALWAYS_TRUE) {
// We will not compile the test expression. Label the top of
- // the loop with the continue target if there is no update
- // expression, otherwise with the body target.
+ // the loop.
if (node->next() == NULL) {
+ // Use the continue target if there is no update expression.
node->continue_target()->Initialize(this,
JumpTarget::BIDIRECTIONAL);
node->continue_target()->Bind();
} else {
+ // Otherwise use the backward loop target.
node->continue_target()->Initialize(this);
- body.Bind();
+ loop.Bind();
}
} else {
ASSERT(info == DONT_KNOW);
- node->continue_target()->Initialize(this);
+ if (test_at_bottom) {
+ // Continue is either the update expression or the test at
+ // the bottom, no need to label the test at the top.
+ node->continue_target()->Initialize(this);
+ } else if (node->next() == NULL) {
+ // We are not recompiling the test at the bottom and there
+ // is no update expression.
+ node->continue_target()->Initialize(this,
JumpTarget::BIDIRECTIONAL);
+ node->continue_target()->Bind();
+ } else {
+ // We are not recompiling the test at the bottom and there
+ // is an update expression.
+ node->continue_target()->Initialize(this);
+ loop.Bind();
+ }
+
// Compile the test with the body as the true target and
// preferred fall-through and with the break target as the
// false target.
@@ -2337,9 +2380,9 @@
LoadCondition(node->cond(), NOT_INSIDE_TYPEOF, &dest, true);
if (dest.false_was_fall_through()) {
- // If we don't have dangling jumps to the body, the test is
- // unconditionally false and we do not need to compile the
- // body.
+ // If we got the break target as fall-through, the test may
+ // have been unconditionally false (if there are no jumps to
+ // the body).
if (!body.is_linked()) break;
// Otherwise, jump around the body on the fall through and
@@ -2350,20 +2393,17 @@
}
}
- // The (stack check at the start of the) body was labeled.
- // Compile it.
CheckStack(); // TODO(1222600): ignore if body contains calls.
Visit(node->body());
// If there is an update expression, compile it if necessary.
if (node->next() != NULL) {
- // We did not use the continue target for the body.
if (node->continue_target()->is_linked()) {
node->continue_target()->Bind();
}
// Control can reach the update by falling out of the body or
- // by a continue in the body.
+ // by a continue.
if (has_valid_frame()) {
// Record the source position of the statement as this code
// which is after the code for the body actually belongs to
@@ -2373,31 +2413,42 @@
}
}
- // Compile the test if necessary and jump back.
+ // Based on the condition analysis, compile the backward jump as
+ // necessary.
if (info == ALWAYS_TRUE) {
if (has_valid_frame()) {
if (node->next() == NULL) {
node->continue_target()->Jump();
} else {
- body.Jump();
+ loop.Jump();
}
}
} else {
ASSERT(info == DONT_KNOW); // ALWAYS_FALSE cannot reach here.
- if (node->continue_target()->is_linked()) {
- // We can have dangling jumps to the continue target if
- // there was no update expression.
- node->continue_target()->Bind();
- }
-
- // Control can reach the test at the bottom by falling out of
- // the body, by a continue in the body, or from the update
- // expression.
- if (has_valid_frame()) {
- // The break target is the fall-through (body is a backward
- // jump from here).
- ControlDestination dest(&body, node->break_target(), false);
- LoadCondition(node->cond(), NOT_INSIDE_TYPEOF, &dest, true);
+ if (test_at_bottom) {
+ if (node->continue_target()->is_linked()) {
+ // We can have dangling jumps to the continue target if
+ // there was no update expression.
+ node->continue_target()->Bind();
+ }
+ // Control can reach the test at the bottom by falling out
+ // of the body, by a continue in the body, or from the
+ // update expression.
+ if (has_valid_frame()) {
+ // The break target is the fall-through (body is a
+ // backward jump from here).
+ ControlDestination dest(&body, node->break_target(), false);
+ LoadCondition(node->cond(), NOT_INSIDE_TYPEOF, &dest, true);
+ }
+ } else {
+ // Otherwise, jump back to the test at the top.
+ if (has_valid_frame()) {
+ if (node->next() == NULL) {
+ node->continue_target()->Jump();
+ } else {
+ loop.Jump();
+ }
+ }
}
}
Added: branches/bleeding_edge/test/mjsunit/regress/regress-260.js
==============================================================================
--- (empty file)
+++ branches/bleeding_edge/test/mjsunit/regress/regress-260.js Mon Mar 9
07:12:20 2009
@@ -0,0 +1,33 @@
+// Copyright 2009 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.
+
+// We should not compile the bodies of function literals in loop
+// conditions twice, even in cases where the loop condition is
+// compiled twice.
+
+function test() { eval("while(!function () { var x; });"); }
+test();
--~--~---------~--~----~------------~-------~--~----~
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
-~----------~----~----~----~------~----~------~--~---
