Author: [email protected]
Date: Wed Feb 25 06:09:48 2009
New Revision: 1361
Modified:
branches/experimental/toiger/src/codegen-ia32.cc
branches/experimental/toiger/src/codegen-ia32.h
Log:
Experimental: fix a bug in our handling of the short-circuit boolean
operators && and ||.
Correctly detect that the left subexpression of && is unconditionally
false and set the state of the inherited control destination; and if
it is not unconditionally false ensure that the right subexpression is
compiled.
Analogously for ||.
Review URL: http://codereview.chromium.org/27130
Modified: branches/experimental/toiger/src/codegen-ia32.cc
==============================================================================
--- branches/experimental/toiger/src/codegen-ia32.cc (original)
+++ branches/experimental/toiger/src/codegen-ia32.cc Wed Feb 25 06:09:48
2009
@@ -4621,18 +4621,35 @@
LoadCondition(node->left(), NOT_INSIDE_TYPEOF, &dest, false);
if (dest.false_was_fall_through()) {
- // We have just bound the false target. There may be dangling
- // jumps to is_true.
+ // The current false target was used as the fall-through. If
+ // there are no dangling jumps to is_true then the left
+ // subexpression was unconditionally false. Otherwise we have
+ // paths where we do have to evaluate the right subexpression.
if (is_true.is_linked()) {
- destination()->false_target()->Unuse();
- destination()->false_target()->Jump();
+ // We need to compile the right subexpression. If the jump to
+ // the current false target was a forward jump then we have a
+ // valid frame, we have just bound the false target, and we
+ // have to jump around the code for the right subexpression.
+ if (has_valid_frame()) {
+ destination()->false_target()->Unuse();
+ destination()->false_target()->Jump();
+ }
is_true.Bind();
- destination()->Goto(true);
+ // The left subexpression compiled to control flow, so the
+ // right one is free to do so as well.
+ LoadCondition(node->right(), NOT_INSIDE_TYPEOF, destination(),
false);
+ } else {
+ // We have actually just jumped to or bound the current false
+ // target but the current control destination is not marked as
+ // used.
+ destination()->Use(false);
}
+
} else if (dest.is_used()) {
- // The first subexpression compiled to control flow (is_true was
- // just bound), so the second is free to do so as well.
+ // The left subexpression compiled to control flow (and is_true
+ // was just bound), so the right is free to do so as well.
LoadCondition(node->right(), NOT_INSIDE_TYPEOF, destination(),
false);
+
} else {
// We have a materialized value on the frame, so we exit with
// one on all paths. There are possibly also jumps to is_true
@@ -4653,7 +4670,7 @@
// Pop the result of evaluating the first part.
frame_->Drop();
- // Evaluate right side expression.
+ // Compile right side expression.
is_true.Bind();
Load(node->right());
@@ -4667,18 +4684,34 @@
LoadCondition(node->left(), NOT_INSIDE_TYPEOF, &dest, false);
if (dest.true_was_fall_through()) {
- // We have just bound the true target. There may be dangling
- // jumps to is_false.
+ // The current true target was used as the fall-through. If
+ // there are no dangling jumps to is_false then the left
+ // subexpression was unconditionally true. Otherwise we have
+ // paths where we do have to evaluate the right subexpression.
if (is_false.is_linked()) {
- destination()->true_target()->Unuse();
- destination()->true_target()->Jump();
+ // We need to compile the right subexpression. If the jump to
+ // the current true target was a forward jump then we have a
+ // valid frame, we have just bound the true target, and we
+ // have to jump around the code for the right subexpression.
+ if (has_valid_frame()) {
+ destination()->true_target()->Unuse();
+ destination()->true_target()->Jump();
+ }
is_false.Bind();
- destination()->Goto(false);
+ // The left subexpression compiled to control flow, so the
+ // right one is free to do so as well.
+ LoadCondition(node->right(), NOT_INSIDE_TYPEOF, destination(),
false);
+ } else {
+ // We have just jumped to or bound the current true target but
+ // the current control destination is not marked as used.
+ destination()->Use(true);
}
+
} else if (dest.is_used()) {
- // The first subexpression compiled to control flow (is_false
- // was just bound), so the second is free to do so as well.
+ // The left subexpression compiled to control flow (and is_false
+ // was just bound), so the right is free to do so as well.
LoadCondition(node->right(), NOT_INSIDE_TYPEOF, destination(),
false);
+
} else {
// We have a materialized value on the frame, so we exit with
// one on all paths. There are possibly also jumps to is_false
@@ -4699,7 +4732,7 @@
// Pop the result of evaluating the first part.
frame_->Drop();
- // Evaluate right side expression.
+ // Compile right side expression.
is_false.Bind();
Load(node->right());
Modified: branches/experimental/toiger/src/codegen-ia32.h
==============================================================================
--- branches/experimental/toiger/src/codegen-ia32.h (original)
+++ branches/experimental/toiger/src/codegen-ia32.h Wed Feb 25 06:09:48 2009
@@ -191,6 +191,18 @@
true_is_fall_through_ = where;
}
+ // Mark this jump target as used as if Goto had been called, but
+ // without generating a jump or binding a label (the control effect
+ // should have already happened). This is used when the left
+ // subexpression of the short-circuit boolean operators are
+ // compiled.
+ void Use(bool where) {
+ ASSERT(!is_used_);
+ ASSERT((where ? true_target_ : false_target_)->is_bound());
+ is_used_ = true;
+ true_is_fall_through_ = where;
+ }
+
// Swap the true and false targets but keep the same actual label as
// the fall through. This is used when compiling negated
// expressions, where we want to swap the targets but preserve the
--~--~---------~--~----~------------~-------~--~----~
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
-~----------~----~----~----~------~----~------~--~---
