Revert of [turbofan] Introduce a dedicated CheckBounds operator. (patchset #5...

Revert of [turbofan] Introduce a dedicated CheckBounds operator. (patchset #5 id:80001 of https://codereview.chromium.org/2035893004/ )

Reason for revert:
Speculative revert since V8 roll is blocked.

Buildbot: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/228171

Example log: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/228171/steps/browser_tests%20%28with%20patch%29%20on%20Ubuntu-12.04/logs/CreateNewFolder_FileManagerBrowserTest.Test_0

Failing assert:
#
# Fatal error in ../../v8/src/compiler/node.cc, line 63
# Node::New() Error: #202:DeoptimizeUnless[1] is nullptr
#

(I take it that's a rather generic assert in TF, hence the revert is somewhat sepculative.)

Original issue's description:
> [turbofan] Introduce a dedicated CheckBounds operator.
>
> This CheckBounds simplified operator is similar to the HBoundsCheck in
> Crankshaft, and is hooked up to the new type feedback support in the
> SimplifiedLowering. We use it to check the index bounds for keyed
> property accesses.
>
> Note to perf sheriffs: This will tank quite a few benchmarks, as the
> operator makes some redundant branch elimination ineffective for
> certain patterns of keyed accesses. This does require more serious
> redundancy elimination, which we will do in a separate CL. So ignore
> any regressions from this CL, we know there will be a few.
>
> R=jarin@chromium.org
> BUG=v8:4470,v8:5100
>
> Committed: https://crrev.com/85e5567dae66a918500ae94c5568221137a0f5d4
> Cr-Commit-Position: refs/heads/master@{#36947}

TBR=jarin@chromium.org,bmeurer@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:4470,v8:5100

Review-Url: https://codereview.chromium.org/2064163002
Cr-Commit-Position: refs/heads/master@{#36975}

src/compiler/effect-control-linearizer.cc

@@ -416,9 +416,6 @@ bool EffectControlLinearizer::TryWireInStateEffect(Node* node,
     case IrOpcode::kTruncateTaggedToFloat64:
       state = LowerTruncateTaggedToFloat64(node, *effect, *control);
       break;
-    case IrOpcode::kCheckBounds:
-      state = LowerCheckBounds(node, frame_state, *effect, *control);
-      break;
     case IrOpcode::kCheckedUint32ToInt32:
       state = LowerCheckedUint32ToInt32(node, frame_state, *effect, *control);
       break;
@@ -761,22 +758,6 @@ EffectControlLinearizer::LowerTruncateTaggedToFloat64(Node* node, Node* effect,
   return ValueEffectControl(value, effect, control);
 }
 
-EffectControlLinearizer::ValueEffectControl
-EffectControlLinearizer::LowerCheckBounds(Node* node, Node* frame_state,
-                                          Node* effect, Node* control) {
-  Node* index = node->InputAt(0);
-  Node* limit = node->InputAt(1);
-
-  Node* check = graph()->NewNode(machine()->Uint32LessThan(), index, limit);
-  control = effect = graph()->NewNode(common()->DeoptimizeUnless(), check,
-                                      frame_state, effect, control);
-
-  // Make sure the lowered node does not appear in any use lists.
-  node->TrimInputCount(0);
-
-  return ValueEffectControl(index, effect, control);
-}
-
 EffectControlLinearizer::ValueEffectControl
 EffectControlLinearizer::LowerCheckedUint32ToInt32(Node* node,
                                                    Node* frame_state,
@@ -865,36 +846,26 @@ EffectControlLinearizer::LowerCheckedTaggedToInt32(Node* node,
 
   // In the non-Smi case, check the heap numberness, load the number and convert
   // to int32.
+  // TODO(jarin) Propagate/handle possible truncations here.
   Node* if_false = graph()->NewNode(common()->IfFalse(), branch);
-  Node* efalse = effect;
-  Node* vfalse;
-  {
-    Node* value_map = efalse =
-        graph()->NewNode(simplified()->LoadField(AccessBuilder::ForMap()),
-                         value, efalse, if_false);
-    Node* check = graph()->NewNode(machine()->WordEqual(), value_map,
-                                   jsgraph()->HeapNumberMapConstant());
-    if_false = efalse = graph()->NewNode(common()->DeoptimizeUnless(), check,
-                                         frame_state, efalse, if_false);
-    vfalse = efalse = graph()->NewNode(
-        simplified()->LoadField(AccessBuilder::ForHeapNumberValue()), value,
-        efalse, if_false);
-    ValueEffectControl state =
-        BuildCheckedFloat64ToInt32(vfalse, frame_state, efalse, if_false);
-    if_false = state.control;
-    efalse = state.effect;
-    vfalse = state.value;
-  }
+  ValueEffectControl number_state = BuildCheckedHeapNumberOrOddballToFloat64(
+      value, frame_state, effect, if_false);
+  number_state =
+      BuildCheckedFloat64ToInt32(number_state.value, frame_state,
+                                 number_state.effect, number_state.control);
 
-  control = graph()->NewNode(common()->Merge(2), if_true, if_false);
-  effect = graph()->NewNode(common()->EffectPhi(2), etrue, efalse, control);
-  value = graph()->NewNode(common()->Phi(MachineRepresentation::kWord32, 2),
-                           vtrue, vfalse, control);
+  Node* merge =
+      graph()->NewNode(common()->Merge(2), if_true, number_state.control);
+  Node* effect_phi = graph()->NewNode(common()->EffectPhi(2), etrue,
+                                      number_state.effect, merge);
+  Node* result =
+      graph()->NewNode(common()->Phi(MachineRepresentation::kWord32, 2), vtrue,
+                       number_state.value, merge);
 
   // Make sure the lowered node does not appear in any use lists.
   node->TrimInputCount(0);
 
-  return ValueEffectControl(value, effect, control);
+  return ValueEffectControl(result, effect_phi, merge);
 }
 
 EffectControlLinearizer::ValueEffectControl

src/compiler/effect-control-linearizer.h

@@ -62,8 +62,6 @@ class EffectControlLinearizer {
                                               Node* control);
   ValueEffectControl LowerChangeTaggedToUint32(Node* node, Node* effect,
                                                Node* control);
-  ValueEffectControl LowerCheckBounds(Node* node, Node* frame_state,
-                                      Node* effect, Node* control);
   ValueEffectControl LowerCheckedUint32ToInt32(Node* node, Node* frame_state,
                                                Node* effect, Node* control);
   ValueEffectControl LowerCheckedFloat64ToInt32(Node* node, Node* frame_state,

src/compiler/js-native-context-specialization.cc

@@ -634,17 +634,6 @@ Reduction JSNativeContextSpecialization::ReduceElementAccess(
               receiver, jsgraph()->HeapConstant(transition_target), context,
               frame_state, transition_effect, transition_control);
         }
-
-        // TODO(turbofan): The effect/control linearization will not find a
-        // FrameState after the StoreField or Call that is generated for the
-        // elements kind transition above. This is because those operatos don't
-        // have the kNoWrite flag on it, even tho they are not JavaScript
-        // observable, but at the same time adding kNoWrite would make them
-        // eliminatable during instruction selection (at least the Call one).
-        transition_effect =
-            graph()->NewNode(common()->Checkpoint(), frame_state,
-                             transition_effect, transition_control);
-
         this_controls.push_back(transition_control);
         this_effects.push_back(transition_effect);
       }
@@ -662,13 +651,6 @@ Reduction JSNativeContextSpecialization::ReduceElementAccess(
         this_effect =
             graph()->NewNode(common()->EffectPhi(this_control_count),
                              this_control_count + 1, &this_effects.front());
-
-        // TODO(turbofan): This is another work-around, which is necessary
-        // in addition to the Checkpoint above, as the CheckpointElimination
-        // is not really compositional. We really need a way to address the
-        // "no-write" problem on non-side-effecting nodes.
-        this_effect = graph()->NewNode(common()->Checkpoint(), frame_state,
-                                       this_effect, this_control);
       }
     }
 
@@ -680,6 +662,30 @@ Reduction JSNativeContextSpecialization::ReduceElementAccess(
       AssumePrototypesStable(receiver_type, native_context, holder);
     }
 
+    // Check that the {index} is actually a Number.
+    if (!NumberMatcher(this_index).HasValue()) {
+      Node* check =
+          graph()->NewNode(simplified()->ObjectIsNumber(), this_index);
+      this_control = this_effect =
+          graph()->NewNode(common()->DeoptimizeUnless(), check, frame_state,
+                           this_effect, this_control);
+      this_index = graph()->NewNode(simplified()->TypeGuard(Type::Number()),
+                                    this_index, this_control);
+    }
+
+    // Convert the {index} to an unsigned32 value and check if the result is
+    // equal to the original {index}.
+    if (!NumberMatcher(this_index).IsInRange(0.0, kMaxUInt32)) {
+      Node* this_index32 =
+          graph()->NewNode(simplified()->NumberToUint32(), this_index);
+      Node* check = graph()->NewNode(simplified()->NumberEqual(), this_index32,
+                                     this_index);
+      this_control = this_effect =
+          graph()->NewNode(common()->DeoptimizeUnless(), check, frame_state,
+                           this_effect, this_control);
+      this_index = this_index32;
+    }
+
     // TODO(bmeurer): We currently specialize based on elements kind. We should
     // also be able to properly support strings and other JSObjects here.
     ElementsKind elements_kind = access_info.elements_kind();
@@ -715,8 +721,10 @@ Reduction JSNativeContextSpecialization::ReduceElementAccess(
                   this_elements, this_effect, this_control);
 
     // Check that the {index} is in the valid range for the {receiver}.
-    this_index = this_effect =
-        graph()->NewNode(simplified()->CheckBounds(), this_index, this_length,
+    Node* check = graph()->NewNode(simplified()->NumberLessThan(), this_index,
+                                   this_length);
+    this_control = this_effect =
+        graph()->NewNode(common()->DeoptimizeUnless(), check, frame_state,
                          this_effect, this_control);
 
     // Compute the element access.

src/compiler/opcodes.h

@@ -220,7 +220,6 @@
   V(ChangeFloat64ToTagged)         \
   V(ChangeTaggedToBit)             \
   V(ChangeBitToTagged)             \
-  V(CheckBounds)                   \
   V(CheckedUint32ToInt32)          \
   V(CheckedFloat64ToInt32)         \
   V(CheckedTaggedToInt32)          \

src/compiler/simplified-lowering.cc

@@ -1575,13 +1575,6 @@ class RepresentationSelector {
         }
         return;
       }
-
-      case IrOpcode::kCheckBounds: {
-        VisitBinop(node, UseInfo::CheckedSigned32AsWord32(),
-                   UseInfo::TruncatingWord32(), MachineRepresentation::kWord32);
-        return;
-      }
-
       case IrOpcode::kAllocate: {
         ProcessInput(node, 0, UseInfo::TruncatingWord32());
         ProcessRemainingInputs(node, 1);

src/compiler/simplified-operator.cc

@@ -334,12 +334,6 @@ const Operator* SimplifiedOperatorBuilder::ReferenceEqual(Type* type) {
                                "ReferenceEqual", 2, 0, 0, 1, 0, 0);
 }
 
-const Operator* SimplifiedOperatorBuilder::CheckBounds() {
-  // TODO(bmeurer): Cache this operator. Make it pure!
-  return new (zone()) Operator(IrOpcode::kCheckBounds, Operator::kEliminatable,
-                               "CheckBounds", 2, 1, 1, 1, 1, 0);
-}
-
 const Operator* SimplifiedOperatorBuilder::TypeGuard(Type* type) {
   class TypeGuardOperator final : public Operator1<Type*> {
    public:

src/compiler/simplified-operator.h

@@ -195,8 +195,6 @@ class SimplifiedOperatorBuilder final : public ZoneObject {
   const Operator* TruncateTaggedToWord32();
   const Operator* TruncateTaggedToFloat64();
 
-  const Operator* CheckBounds();
-
   const Operator* CheckedUint32ToInt32();
   const Operator* CheckedFloat64ToInt32();
   const Operator* CheckedTaggedToInt32();

src/compiler/typer.cc

@@ -1954,11 +1954,6 @@ Type* Typer::Visitor::TypeChangeBitToTagged(Node* node) {
   return ChangeRepresentation(arg, Type::TaggedPointer(), zone());
 }
 
-Type* Typer::Visitor::TypeCheckBounds(Node* node) {
-  // TODO(bmeurer): We could do better here based on the limit.
-  return Type::Unsigned31();
-}
-
 Type* Typer::Visitor::TypeCheckedUint32ToInt32(Node* node) {
   return Type::Signed32();
 }

src/compiler/verifier.cc

@@ -933,12 +933,6 @@ void Verifier::Visitor::Check(Node* node) {
       break;
     }
 
-    case IrOpcode::kCheckBounds:
-      CheckValueInputIs(node, 0, Type::Any());
-      CheckValueInputIs(node, 1, Type::Unsigned31());
-      CheckUpperIs(node, Type::Unsigned31());
-      break;
-
     case IrOpcode::kCheckedUint32ToInt32:
     case IrOpcode::kCheckedFloat64ToInt32:
     case IrOpcode::kCheckedTaggedToInt32:

test/mjsunit/compiler/regress-5100.js

-// Copyright 2016 the V8 project authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-// Flags: --allow-natives-syntax
-
-var a = [0, 1];
-a["true"] = "true";
-a["false"] = "false";
-a["null"] = "null";
-a["undefined"] = "undefined";
-
-// Ensure we don't accidentially truncate true when used to index arrays.
-(function() {
-  function f(x) { return a[x]; }
-
-  assertEquals(0, f(0));
-  assertEquals(0, f(0));
-  %OptimizeFunctionOnNextCall(f);
-  assertEquals("true", f(true));
-})();
-
-// Ensure we don't accidentially truncate false when used to index arrays.
-(function() {
-  function f( x) { return a[x]; }
-
-  assertEquals(0, f(0));
-  assertEquals(0, f(0));
-  %OptimizeFunctionOnNextCall(f);
-  assertEquals("false", f(false));
-})();
-
-// Ensure we don't accidentially truncate null when used to index arrays.
-(function() {
-  function f( x) { return a[x]; }
-
-  assertEquals(0, f(0));
-  assertEquals(0, f(0));
-  %OptimizeFunctionOnNextCall(f);
-  assertEquals("null", f(null));
-})();
-
-// Ensure we don't accidentially truncate undefined when used to index arrays.
-(function() {
-  function f( x) { return a[x]; }
-
-  assertEquals(0, f(0));
-  assertEquals(0, f(0));
-  %OptimizeFunctionOnNextCall(f);
-  assertEquals("undefined", f(undefined));
-})();