Reviewers: William Hesse,
Description:
Fix a bug in the placement of minus-zero checks and in GVN.
1. The placement of checks for negative zero has to be computed after
all conversion instructions have been inserted. I separated the code
into its own phase.
2. GVN need to take instruction flags into account when comparing
instructions for redundancy.
Please review this at http://codereview.chromium.org/6260035/
SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge/
Affected files:
M src/hydrogen-instructions.cc
M src/hydrogen.h
M src/hydrogen.cc
A test/mjsunit/compiler/regress-1085.js
Index: src/hydrogen-instructions.cc
===================================================================
--- src/hydrogen-instructions.cc (revision 6529)
+++ src/hydrogen-instructions.cc (working copy)
@@ -253,6 +253,7 @@
if (other->opcode() != opcode()) return false;
if (!other->representation().Equals(representation())) return false;
if (!other->type_.Equals(type_)) return false;
+ if (other->flags() != flags()) return false;
if (OperandCount() != other->OperandCount()) return false;
for (int i = 0; i < OperandCount(); ++i) {
if (OperandAt(i)->id() != other->OperandAt(i)->id()) return false;
Index: src/hydrogen.cc
===================================================================
--- src/hydrogen.cc (revision 6529)
+++ src/hydrogen.cc (working copy)
@@ -1813,15 +1813,6 @@
HValue* use,
Representation to,
bool is_truncating) {
- // Propagate flags for negative zero checks upwards from conversions
- // int32-to-tagged and int32-to-double.
- Representation from = value->representation();
- if (from.IsInteger32()) {
- ASSERT(to.IsTagged() || to.IsDouble());
- BitVector visited(GetMaximumValueID());
- PropagateMinusZeroChecks(value, &visited);
- }
-
// Insert the representation change right before its use. For phi-uses we
// insert at the end of the corresponding predecessor.
HBasicBlock* insert_block = use->block();
@@ -1984,6 +1975,28 @@
}
+void HGraph::ComputeMinusZeroChecks() {
+ for (int i = 0; i < blocks_.length(); ++i) {
+ for (HInstruction* current = blocks_[i]->first();
+ current != NULL;
+ current = current->next()) {
+ if (current->IsChange()) {
+ HChange* change = HChange::cast(current);
+ // Propagate flags for negative zero checks upwards from
conversions
+ // int32-to-tagged and int32-to-double.
+ Representation from = change->value()->representation();
+ ASSERT(from.Equals(change->from()));
+ if (from.IsInteger32()) {
+ ASSERT(change->to().IsTagged() || change->to().IsDouble());
+ BitVector visited(GetMaximumValueID());
+ PropagateMinusZeroChecks(change->value(), &visited);
+ }
+ }
+ }
+ }
+}
+
+
// Implementation of utility classes to represent an expression's context
in
// the AST.
AstContext::AstContext(HGraphBuilder* owner, Expression::Context kind)
@@ -2243,6 +2256,7 @@
graph_->InitializeInferredTypes();
graph_->Canonicalize();
graph_->InsertRepresentationChanges();
+ graph_->ComputeMinusZeroChecks();
// Eliminate redundant stack checks on backwards branches.
HStackCheckEliminator sce(graph_);
Index: src/hydrogen.h
===================================================================
--- src/hydrogen.h (revision 6529)
+++ src/hydrogen.h (working copy)
@@ -307,6 +307,7 @@
void InitializeInferredTypes();
void InsertTypeConversions();
void InsertRepresentationChanges();
+ void ComputeMinusZeroChecks();
bool ProcessArgumentsObject();
void EliminateRedundantPhis();
void Canonicalize();
Index: test/mjsunit/compiler/regress-1085.js
===================================================================
--- test/mjsunit/compiler/regress-1085.js (revision 0)
+++ test/mjsunit/compiler/regress-1085.js (revision 0)
@@ -0,0 +1,35 @@
+// 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.
+
+
+// Test correct checks for negative zero.
+// This test relies on specific type feedback for Math.min.
+function f(x) { return 1 / Math.min(1, x); }
+
+for (var i=0; i<1000000; i++) f(1);
+
+assertEquals(-Infinity, f(-0));
Property changes on: test/mjsunit/compiler/regress-1085.js
___________________________________________________________________
Added: svn:eol-style
+ LF
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
