LGTM. Please file a bug on fixing this using dependencies and add TODO comments near the new flags to explain that they are there as a workaround. On Mar 24, 2011 1:22 PM, <[email protected]> wrote: > Reviewers: Vitaly Repeshko, Kevin Millikin, > > Description: > Fix bug that caused invalid code motion for certain loads instructions. > > The dependency flags of instructions depending on a previous check have to > be a super-set of the flags of the check instructions. > > Please review this at http://codereview.chromium.org/6730025/ > > SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge/ > > Affected files: > M src/hydrogen-instructions.h > A test/mjsunit/compiler/regress-loadfield.js > > > Index: src/hydrogen-instructions.h > =================================================================== > --- src/hydrogen-instructions.h (revision 7333) > +++ src/hydrogen-instructions.h (working copy) > @@ -1407,8 +1407,9 @@ > // object. It is guaranteed to be 32 bit integer, but it can be > // represented as either a smi or heap number. > set_representation(Representation::Tagged()); > + SetFlag(kUseGVN); > SetFlag(kDependsOnArrayLengths); > - SetFlag(kUseGVN); > + SetFlag(kDependsOnMaps); > } > > virtual Representation RequiredInputRepresentation(int index) const { > @@ -1426,8 +1427,8 @@ > public: > explicit HFixedArrayLength(HValue* value) : HUnaryOperation(value) { > set_representation(Representation::Tagged()); > + SetFlag(kUseGVN); > SetFlag(kDependsOnArrayLengths); > - SetFlag(kUseGVN); > } > > virtual Representation RequiredInputRepresentation(int index) const { > @@ -2257,6 +2258,7 @@ > : HBinaryOperation(left, right) { > set_representation(Representation::Tagged()); > SetFlag(kUseGVN); > + SetFlag(kDependsOnMaps); > } > > virtual bool EmitAtUses() { > @@ -2942,6 +2944,7 @@ > offset_(offset) { > set_representation(Representation::Tagged()); > SetFlag(kUseGVN); > + SetFlag(kDependsOnMaps); > if (is_in_object) { > SetFlag(kDependsOnInobjectFields); > } else { > @@ -3268,6 +3271,7 @@ > : HBinaryOperation(string, index) { > set_representation(Representation::Integer32()); > SetFlag(kUseGVN); > + SetFlag(kDependsOnMaps); > } > > virtual Representation RequiredInputRepresentation(int index) const { > @@ -3312,6 +3316,7 @@ > explicit HStringLength(HValue* string) : HUnaryOperation(string) { > set_representation(Representation::Tagged()); > SetFlag(kUseGVN); > + SetFlag(kDependsOnMaps); > } > > virtual Representation RequiredInputRepresentation(int index) const { > Index: test/mjsunit/compiler/regress-loadfield.js > =================================================================== > --- test/mjsunit/compiler/regress-loadfield.js (revision 0) > +++ test/mjsunit/compiler/regress-loadfield.js (revision 0) > @@ -0,0 +1,65 @@ > +// 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. > + > +// Regression test for GVN on field loads. > + > +function bar() {} > + > +// Make sure there is a transition on adding "bar" inobject property. > +var b = new bar(); > +b.bar = "bar"; > + > +function test(a) { > + var b = new Array(10); > + for (var i = 0; i < 10; i++) { > + b[i] = new bar(); > + } > + > + for (var i = 0; i < 10; i++) { > + b[i].bar = a.foo; > + } > +} > + > +// Create an object with fast backing store properties. > +var a = {}; > +a.p1 = ""; > +a.p2 = ""; > +a.p3 = ""; > +a.p4 = ""; > +a.p5 = ""; > +a.p6 = ""; > +a.p7 = ""; > +a.p8 = ""; > +a.p9 = ""; > +a.p10 = ""; > +a.p11 = ""; > +a.foo = "foo"; > +for (var i = 0; i < 100000; i++) { > + test(a); > +} > + > +test(""); > > Property changes on: test/mjsunit/compiler/regress-loadfield.js > ___________________________________________________________________ > Added: svn:eol-style > + LF > > >
-- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
