Improve GVN handling of ElementTransitions.

test/mjsunit/elements-transition-hoisting.js

+// 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.
+
+// Flags: --allow-natives-syntax --smi-only-arrays
+
+// Ensure that ElementsKind transitions in various situations are hoisted (or
+// not hoisted) correctly, don't change the semantics programs and don't trigger
+// deopt through hoisting in important situations.
+
+support_smi_only_arrays = %HasFastSmiOnlyElements(new Array(1,2,3,4,5,6));
+
+if (support_smi_only_arrays) {
+  print("Tests include smi-only arrays.");
+} else {
+  print("Tests do NOT include smi-only arrays.");
+}
+
+var global_false = false;
+
+if (support_smi_only_arrays) {
+  // Make sure that multiple stores to an object array get hoisted in a way that

[fschneider, 2012/02/07 10:05:05]

What is meant by hoisting multiple stores? Stores themselves never get hoisted.

[danno, 2012/02/08 17:18:42]

Done.

+  // doesn't generate a deopt in simple cases.
+  function testDoubleConversion4(a) {
+    var object = new Object();
+    a[0] = 0;
+    do {
+      a[0] = object;
+    } while (global_false);

[fschneider, 2012/02/07 10:05:05]

Maybe you should go around the loop >1 times in all tests where you want to test
that hoisting does not cause deopts.

[danno, 2012/02/08 17:18:42]

Done.

+  }
+
+  testDoubleConversion4(new Array(5));
+  %OptimizeFunctionOnNextCall(testDoubleConversion4);
+  testDoubleConversion4(new Array(5));
+  testDoubleConversion4(new Array(5));
+  assertTrue(2 != %GetOptimizationStatus(testDoubleConversion4));
+
+  // Make sure that non-element related map checks still get hoisted in a way
+  // that doesn't generate a deopt in simple cases.
+  function testExactMapHoisting(a) {
+    var object = new Object();
+    a.foo = 0;
+    a[0] = 0;
+    a[1] = 1;
+    do {
+      a.foo = object; // This map check should be hoistable
+      a[1] = object;
+      result = a.foo == object && a[1] == object;
+    } while (global_false);
+  }
+
+  testExactMapHoisting(new Array(5));
+  %OptimizeFunctionOnNextCall(testExactMapHoisting);
+  testExactMapHoisting(new Array(5));
+  testExactMapHoisting(new Array(5));
+  assertTrue(2 != %GetOptimizationStatus(testExactMapHoisting));
+
+  // Make sure that non-element related map checks still get hoisted if all
+  // known element transitions have been done by the time the map check is done.
+  // Make sure it's done in a way that doesn't deopt later.
+  function testExactMapHoisting2(a) {
+    var object = new Object();
+    a.foo = 0;
+    a[0] = 0;
+    a[1] = 1;
+    do {
+      a[1] = 2.5;
+      a.foo = object; // This map check should be hoistable
+    } while (global_false);
+  }
+
+  testExactMapHoisting2(new Array(5));
+  %OptimizeFunctionOnNextCall(testExactMapHoisting2);
+  testExactMapHoisting2(new Array(5));
+  testExactMapHoisting2(new Array(5));
+  assertTrue(2 != %GetOptimizationStatus(testExactMapHoisting2));
+
+  // Make sure that non-element related map checks still get hoisted if all

[fschneider, 2012/02/07 10:05:05]

The description of this test does not match the comment below where you write
that the map check should not be hoisted.

[danno, 2012/02/08 17:18:42]

Done.

+  // known element transitions have been done by the time the map check is done.
+  // Make sure it's done in a way that doesn't deopt later.
+  function testExactMapHoisting3(a) {
+    var object = new Object();
+    a.foo = 0;
+    a[0] = 0;
+    a[1] = 1;
+    do {
+      a[1] = 2.5;
+      a.foo = object; // This map check should NOT be hoistable because includes

[fschneider, 2012/02/07 10:05:05]

When running this test I see this map check hoisted which contradicts your
expectation here.

[danno, 2012/02/08 17:18:42]

Done.

+                      // a check for the FAST_ELEMENTS map as well as the
+                      // FAST_DOUBLE_ELEMENTS map.
+    } while (global_false);
+  }
+
+  var add_transition = new Array(5);
+  add_transition.foo = 0;
+  add_transition[0] = new Object(); // For FAST_ELEMENT transition to be created
+  testExactMapHoisting3(new Array(5));
+  %OptimizeFunctionOnNextCall(testExactMapHoisting3);
+  testExactMapHoisting3(new Array(5));
+  testExactMapHoisting3(new Array(5));
+  assertTrue(2 != %GetOptimizationStatus(testExactMapHoisting3));
+
+  function testDominatingTransitionHoisting1(a) {
+    var object = new Object();
+    a.foo = 0;

[fschneider, 2012/02/07 10:05:05]

Since a.foo is not used in the loop, I don't see why it's necessary to add it
here.

[danno, 2012/02/08 17:18:42]

Done.

+    a[0] = 0;
+    do {
+      if (global_false != true) {

[fschneider, 2012/02/07 10:05:05]

if (!global_false)

Otherwise the compare is generic (and a call inside the loop).

[danno, 2012/02/08 17:18:42]

Done.

+        a[1] = 2.5;
+      }
+      a[0] = object;
+    } while (global_false);
+  }
+
+  testDominatingTransitionHoisting1(new Array(5));
+  %OptimizeFunctionOnNextCall(testDominatingTransitionHoisting1);
+  testDominatingTransitionHoisting1(new Array(5));
+  testDominatingTransitionHoisting1(new Array(5));
+  assertTrue(2 != %GetOptimizationStatus(testDominatingTransitionHoisting1));
+
+  function testHoistingWithSideEffect(a) {
+    var object = new Object();
+    a.foo = 0;

[fschneider, 2012/02/07 10:05:05]

Since a.foo is not used in the loop, I don't see why it's necessary to add it
here.

[danno, 2012/02/08 17:18:42]

Done.

+    a[0] = 0;
+    do {
+      assertTrue(true);
+      a[0] = object;
+    } while (global_false);
+  }
+
+  testHoistingWithSideEffect(new Array(5));
+  %OptimizeFunctionOnNextCall(testHoistingWithSideEffect);
+  testHoistingWithSideEffect(new Array(5));
+  testHoistingWithSideEffect(new Array(5));
+  assertTrue(2 != %GetOptimizationStatus(testHoistingWithSideEffect));
+}