[interpreter] Add bytecodes for JumpIfNotHole with constant

Adds JumpIfNotHoleConstant and JumpIfNotHoleConstantWide bytecodes
and removes JumpIfHole bytecode.

In situations with large numbers of constants, the generator would
fail because an 8-bit constant could not be reserved for
JumpIfHole/JumpIfNotHole and so a 16-bit constant would be reserved.
Then when patching the bytecode the patcher would discover there was
no wide constant variant of the emitted jump.

BUG=v8:4280,v8:4680
LOG=N

Review URL: https://codereview.chromium.org/1697473002

Cr-Commit-Position: refs/heads/master@{#33952}

src/compiler/bytecode-graph-builder.cc

@@ -1426,6 +1426,16 @@ void BytecodeGraphBuilder::VisitJumpIfToBooleanFalseConstantWide() {
   BuildJumpIfToBooleanEqual(jsgraph()->FalseConstant());
 }
 
+void BytecodeGraphBuilder::VisitJumpIfNotHole() { BuildJumpIfNotHole(); }
+
+void BytecodeGraphBuilder::VisitJumpIfNotHoleConstant() {
+  BuildJumpIfNotHole();
+}
+
+void BytecodeGraphBuilder::VisitJumpIfNotHoleConstantWide() {
+  BuildJumpIfNotHole();
+}
+
 void BytecodeGraphBuilder::VisitJumpIfNull() {
   BuildJumpIfEqual(jsgraph()->NullConstant());
 }
@@ -1450,20 +1460,6 @@ void BytecodeGraphBuilder::VisitJumpIfUndefinedConstantWide() {
   BuildJumpIfEqual(jsgraph()->UndefinedConstant());
 }
 
-void BytecodeGraphBuilder::VisitJumpIfHole() {
-  BuildJumpIfEqual(jsgraph()->TheHoleConstant());
-}
-
-void BytecodeGraphBuilder::VisitJumpIfNotHole() {
-  Node* accumulator = environment()->LookupAccumulator();
-  Node* condition = NewNode(javascript()->StrictEqual(), accumulator,
-                            jsgraph()->TheHoleConstant());
-  Node* node =
-      NewNode(common()->Select(MachineRepresentation::kTagged), condition,
-              jsgraph()->FalseConstant(), jsgraph()->TrueConstant());
-  BuildConditionalJump(node);
-}
-
 void BytecodeGraphBuilder::VisitStackCheck() {
   FrameStateBeforeAndAfter states(this);
   Node* node = NewNode(javascript()->StackCheck());
@@ -1601,6 +1597,15 @@ void BytecodeGraphBuilder::BuildJumpIfToBooleanEqual(Node* comperand) {
   BuildConditionalJump(condition);
 }
 
+void BytecodeGraphBuilder::BuildJumpIfNotHole() {
+  Node* accumulator = environment()->LookupAccumulator();
+  Node* condition = NewNode(javascript()->StrictEqual(), accumulator,
+                            jsgraph()->TheHoleConstant());
+  Node* node =
+      NewNode(common()->Select(MachineRepresentation::kTagged), condition,
+              jsgraph()->FalseConstant(), jsgraph()->TrueConstant());
+  BuildConditionalJump(node);
+}
 
 Node** BytecodeGraphBuilder::EnsureInputBufferSize(int size) {
   if (size > input_buffer_size_) {

src/compiler/bytecode-graph-builder.h

@@ -141,6 +141,7 @@ class BytecodeGraphBuilder {
   void BuildConditionalJump(Node* condition);
   void BuildJumpIfEqual(Node* comperand);
   void BuildJumpIfToBooleanEqual(Node* boolean_comperand);
+  void BuildJumpIfNotHole();
 
   // Simulates control flow by forward-propagating environments.
   void MergeIntoSuccessorEnvironment(int target_offset);

src/interpreter/bytecode-array-builder.cc

@@ -761,6 +761,8 @@ Bytecode BytecodeArrayBuilder::GetJumpWithConstantOperand(
       return Bytecode::kJumpIfToBooleanTrueConstant;
     case Bytecode::kJumpIfToBooleanFalse:
       return Bytecode::kJumpIfToBooleanFalseConstant;
+    case Bytecode::kJumpIfNotHole:
+      return Bytecode::kJumpIfNotHoleConstant;
     case Bytecode::kJumpIfNull:
       return Bytecode::kJumpIfNullConstant;
     case Bytecode::kJumpIfUndefined:
@@ -786,6 +788,8 @@ Bytecode BytecodeArrayBuilder::GetJumpWithConstantWideOperand(
       return Bytecode::kJumpIfToBooleanTrueConstantWide;
     case Bytecode::kJumpIfToBooleanFalse:
       return Bytecode::kJumpIfToBooleanFalseConstantWide;
+    case Bytecode::kJumpIfNotHole:
+      return Bytecode::kJumpIfNotHoleConstantWide;
     case Bytecode::kJumpIfNull:
       return Bytecode::kJumpIfNullConstantWide;
     case Bytecode::kJumpIfUndefined:
@@ -803,7 +807,6 @@ Bytecode BytecodeArrayBuilder::GetJumpWithToBoolean(Bytecode jump_bytecode) {
     case Bytecode::kJump:
     case Bytecode::kJumpIfNull:
     case Bytecode::kJumpIfUndefined:
-    case Bytecode::kJumpIfHole:
     case Bytecode::kJumpIfNotHole:
       return jump_bytecode;
     case Bytecode::kJumpIfTrue:
@@ -970,10 +973,6 @@ BytecodeArrayBuilder& BytecodeArrayBuilder::StackCheck() {
   return *this;
 }
 
-BytecodeArrayBuilder& BytecodeArrayBuilder::JumpIfHole(BytecodeLabel* label) {
-  return OutputJump(Bytecode::kJumpIfHole, label);
-}
-
 BytecodeArrayBuilder& BytecodeArrayBuilder::JumpIfNotHole(
     BytecodeLabel* label) {
   return OutputJump(Bytecode::kJumpIfNotHole, label);

src/interpreter/bytecode-array-builder.h

@@ -225,10 +225,9 @@ class BytecodeArrayBuilder final : public ZoneObject, private RegisterMover {
   BytecodeArrayBuilder& Jump(BytecodeLabel* label);
   BytecodeArrayBuilder& JumpIfTrue(BytecodeLabel* label);
   BytecodeArrayBuilder& JumpIfFalse(BytecodeLabel* label);
+  BytecodeArrayBuilder& JumpIfNotHole(BytecodeLabel* label);
   BytecodeArrayBuilder& JumpIfNull(BytecodeLabel* label);
   BytecodeArrayBuilder& JumpIfUndefined(BytecodeLabel* label);
-  BytecodeArrayBuilder& JumpIfHole(BytecodeLabel* label);
-  BytecodeArrayBuilder& JumpIfNotHole(BytecodeLabel* label);
 
   BytecodeArrayBuilder& StackCheck();
 

src/interpreter/bytecode-generator.cc

@@ -1698,9 +1698,7 @@ void BytecodeGenerator::BuildHoleCheckForVariableLoad(VariableMode mode,
                                                       Handle<String> name) {
   if (mode == CONST_LEGACY) {
     BytecodeLabel end_label;
-    builder()->JumpIfNotHole(&end_label);
-    builder()->LoadUndefined();
-    builder()->Bind(&end_label);
+    builder()->JumpIfNotHole(&end_label).LoadUndefined().Bind(&end_label);
   } else if (mode == LET || mode == CONST) {
     BuildThrowIfHole(name);
   }
@@ -1770,14 +1768,12 @@ void BytecodeGenerator::VisitVariableLoad(Variable* variable,
   }
 }
 
-
 void BytecodeGenerator::VisitVariableLoadForAccumulatorValue(
     Variable* variable, FeedbackVectorSlot slot, TypeofMode typeof_mode) {
   AccumulatorResultScope accumulator_result(this);
   VisitVariableLoad(variable, slot, typeof_mode);
 }
 
-
 Register BytecodeGenerator::VisitVariableLoadForRegisterValue(
     Variable* variable, FeedbackVectorSlot slot, TypeofMode typeof_mode) {
   RegisterResultScope register_scope(this);
@@ -1785,43 +1781,42 @@ Register BytecodeGenerator::VisitVariableLoadForRegisterValue(
   return register_scope.ResultRegister();
 }
 
-void BytecodeGenerator::BuildThrowIfHole(Handle<String> name) {
+void BytecodeGenerator::BuildThrowReferenceError(Handle<String> name) {
+  RegisterAllocationScope register_scope(this);
   Register name_reg = register_allocator()->NewRegister();
-  BytecodeLabel end_label;
-  // TODO(mythria): This will be replaced by a new bytecode that throws an
-  // error if the value is the hole.
-  builder()
-      ->JumpIfNotHole(&end_label)
-      .LoadLiteral(name)
-      .StoreAccumulatorInRegister(name_reg)
-      .CallRuntime(Runtime::kThrowReferenceError, name_reg, 1)
-      .Bind(&end_label);
+  builder()->LoadLiteral(name).StoreAccumulatorInRegister(name_reg).CallRuntime(
+      Runtime::kThrowReferenceError, name_reg, 1);
+}
+
+void BytecodeGenerator::BuildThrowIfHole(Handle<String> name) {
+  // TODO(interpreter): Can the parser reduce the number of checks
+  // performed? Or should there be a ThrowIfHole bytecode.
+  BytecodeLabel no_reference_error;
+  builder()->JumpIfNotHole(&no_reference_error);
+  BuildThrowReferenceError(name);
+  builder()->Bind(&no_reference_error);
 }
 
 void BytecodeGenerator::BuildThrowIfNotHole(Handle<String> name) {
-  Register name_reg = register_allocator()->NewRegister();
-  BytecodeLabel end_label;
-  // TODO(mythria): This will be replaced by a new bytecode that throws an
-  // error if the value is not the hole.
+  // TODO(interpreter): Can the parser reduce the number of checks
+  // performed? Or should there be a ThrowIfNotHole bytecode.
+  BytecodeLabel no_reference_error, reference_error;
   builder()
-      ->JumpIfHole(&end_label)
-      .LoadLiteral(name)
-      .StoreAccumulatorInRegister(name_reg)
-      .CallRuntime(Runtime::kThrowReferenceError, name_reg, 1)
-      .Bind(&end_label);
+      ->JumpIfNotHole(&reference_error)
+      .Jump(&no_reference_error)
+      .Bind(&reference_error);
+  BuildThrowReferenceError(name);
+  builder()->Bind(&no_reference_error);
 }
 
 void BytecodeGenerator::BuildThrowReassignConstant(Handle<String> name) {
-  Register name_reg = register_allocator()->NewRegister();
-  BytecodeLabel else_label;
   // TODO(mythria): This will be replaced by a new bytecode that throws an
   // appropriate error depending on the whether the value is a hole or not.
+  BytecodeLabel const_assign_error;
+  builder()->JumpIfNotHole(&const_assign_error);
+  BuildThrowReferenceError(name);
   builder()
-      ->JumpIfNotHole(&else_label)
-      .LoadLiteral(name)
-      .StoreAccumulatorInRegister(name_reg)
-      .CallRuntime(Runtime::kThrowReferenceError, name_reg, 1)
-      .Bind(&else_label)
+      ->Bind(&const_assign_error)
       .CallRuntime(Runtime::kThrowConstAssignError, Register(), 0);
 }
 
@@ -1841,7 +1836,7 @@ void BytecodeGenerator::BuildHoleCheckForVariableAssignment(Variable* variable,
     // Perform an initialization check for 'this'. 'this' variable is the
     // only variable able to trigger bind operations outside the TDZ
     // via 'super' calls.
-    BuildThrowIfNotHole(variable->name());
+    BuildThrowIfHole(variable->name());
   }
 }
 

src/interpreter/bytecode-generator.h

@@ -78,9 +78,11 @@ class BytecodeGenerator final : public AstVisitor {
                                     TypeofMode typeof_mode = NOT_INSIDE_TYPEOF);
   void VisitVariableAssignment(Variable* variable, Token::Value op,
                                FeedbackVectorSlot slot);
+
   void BuildThrowIfHole(Handle<String> name);
   void BuildThrowIfNotHole(Handle<String> name);
   void BuildThrowReassignConstant(Handle<String> name);
+  void BuildThrowReferenceError(Handle<String> name);
   void BuildHoleCheckForVariableLoad(VariableMode mode, Handle<String> name);
   void BuildHoleCheckForVariableAssignment(Variable* variable, Token::Value op);
 

src/interpreter/bytecodes.cc

@@ -195,10 +195,9 @@ bool Bytecodes::IsConditionalJumpImmediate(Bytecode bytecode) {
          bytecode == Bytecode::kJumpIfFalse ||
          bytecode == Bytecode::kJumpIfToBooleanTrue ||
          bytecode == Bytecode::kJumpIfToBooleanFalse ||
+         bytecode == Bytecode::kJumpIfNotHole ||
          bytecode == Bytecode::kJumpIfNull ||
-         bytecode == Bytecode::kJumpIfUndefined ||
-         bytecode == Bytecode::kJumpIfHole ||
-         bytecode == Bytecode::kJumpIfNotHole;
+         bytecode == Bytecode::kJumpIfUndefined;
 }
 
 
@@ -208,6 +207,7 @@ bool Bytecodes::IsConditionalJumpConstant(Bytecode bytecode) {
          bytecode == Bytecode::kJumpIfFalseConstant ||
          bytecode == Bytecode::kJumpIfToBooleanTrueConstant ||
          bytecode == Bytecode::kJumpIfToBooleanFalseConstant ||
+         bytecode == Bytecode::kJumpIfNotHoleConstant ||
          bytecode == Bytecode::kJumpIfNullConstant ||
          bytecode == Bytecode::kJumpIfUndefinedConstant;
 }
@@ -219,6 +219,7 @@ bool Bytecodes::IsConditionalJumpConstantWide(Bytecode bytecode) {
          bytecode == Bytecode::kJumpIfFalseConstantWide ||
          bytecode == Bytecode::kJumpIfToBooleanTrueConstantWide ||
          bytecode == Bytecode::kJumpIfToBooleanFalseConstantWide ||
+         bytecode == Bytecode::kJumpIfNotHoleConstantWide ||
          bytecode == Bytecode::kJumpIfNullConstantWide ||
          bytecode == Bytecode::kJumpIfUndefinedConstantWide;
 }

src/interpreter/bytecodes.h

@@ -248,9 +248,9 @@ namespace interpreter {
   V(JumpIfUndefined, OperandType::kImm8)                                       \
   V(JumpIfUndefinedConstant, OperandType::kIdx8)                               \
   V(JumpIfUndefinedConstantWide, OperandType::kIdx16)                          \
-  /* TODO(mythria): Replace with opcodes that throw on a hole */               \
-  V(JumpIfHole, OperandType::kImm8)                                            \
   V(JumpIfNotHole, OperandType::kImm8)                                         \
+  V(JumpIfNotHoleConstant, OperandType::kIdx8)                                 \
+  V(JumpIfNotHoleConstantWide, OperandType::kIdx16)                            \
                                                                                \
   /* Complex flow control For..in */                                           \
   V(ForInPrepare, OperandType::kRegOutTriple8)                                 \

src/interpreter/interpreter.cc

@@ -1580,28 +1580,38 @@ void Interpreter::DoJumpIfUndefinedConstantWide(
   DoJumpIfUndefinedConstant(assembler);
 }
 
-// JumpIfHole <imm8>
+// JumpIfNotHole <imm8>
 //
 // Jump by number of bytes represented by an immediate operand if the object
 // referenced by the accumulator is the hole.
-void Interpreter::DoJumpIfHole(InterpreterAssembler* assembler) {
+void Interpreter::DoJumpIfNotHole(InterpreterAssembler* assembler) {
   Node* accumulator = __ GetAccumulator();
   Node* the_hole_value = __ HeapConstant(isolate_->factory()->the_hole_value());
   Node* relative_jump = __ BytecodeOperandImm(0);
-  __ JumpIfWordEqual(accumulator, the_hole_value, relative_jump);
+  __ JumpIfWordNotEqual(accumulator, the_hole_value, relative_jump);
 }
 
-// JumpIfNotHole <imm8>
+// JumpIfNotHoleConstant <idx8>
 //
-// Jump by number of bytes represented by an immediate operand if the object
-// referenced by the accumulator is not the hole.
-void Interpreter::DoJumpIfNotHole(InterpreterAssembler* assembler) {
+// Jump by number of bytes in the Smi in the |idx8| entry in the constant pool
+// if the object referenced by the accumulator is the hole constant.
+void Interpreter::DoJumpIfNotHoleConstant(InterpreterAssembler* assembler) {
   Node* accumulator = __ GetAccumulator();
   Node* the_hole_value = __ HeapConstant(isolate_->factory()->the_hole_value());
-  Node* relative_jump = __ BytecodeOperandImm(0);
+  Node* index = __ BytecodeOperandIdx(0);
+  Node* constant = __ LoadConstantPoolEntry(index);
+  Node* relative_jump = __ SmiUntag(constant);
   __ JumpIfWordNotEqual(accumulator, the_hole_value, relative_jump);
 }
 
+// JumpIfNotHoleConstantWide <idx16>
+//
+// Jump by number of bytes in the Smi in the |idx16| entry in the constant pool
+// if the object referenced by the accumulator is the hole constant.
+void Interpreter::DoJumpIfNotHoleConstantWide(InterpreterAssembler* assembler) {
+  DoJumpIfNotHoleConstant(assembler);
+}
+
 void Interpreter::DoCreateLiteral(Runtime::FunctionId function_id,
                                   InterpreterAssembler* assembler) {
   Node* index = __ BytecodeOperandIdx(0);

test/mjsunit/mjsunit.status

@@ -878,10 +878,6 @@
   # BytecodeGenerator::VisitFunctionLiteral - !shared_info.is_null().
   'regress/regress-crbug-429159': [FAIL],
 
-  # TODO(rmcilroy,4680): Hits unreachable in
-  # BytecodeArrayBuilder::GetJumpWithConstantWideOperand().
-  'regress/regress-455207': [FAIL],
-
   # TODO(rmcilroy,4680): Check failed: osr_normal_entry.
   'regress/regress-123919': [FAIL],
 

test/unittests/interpreter/bytecode-array-builder-unittest.cc

@@ -163,8 +163,8 @@ TEST_F(BytecodeArrayBuilderTest, AllBytecodesGenerated) {
   builder.Jump(&start)
       .JumpIfNull(&start)
       .JumpIfUndefined(&start)
-      .JumpIfHole(&start)
       .JumpIfNotHole(&start);
+
   // Perform an operation that returns boolean value to
   // generate JumpIfTrue/False
   builder.CompareOperation(Token::Value::EQ, reg, Strength::WEAK)
@@ -182,7 +182,8 @@ TEST_F(BytecodeArrayBuilderTest, AllBytecodesGenerated) {
     builder.LoadTrue();
   }
   // Longer jumps requiring Constant operand
-  builder.Jump(&start).JumpIfNull(&start).JumpIfUndefined(&start);
+  builder.Jump(&start).JumpIfNull(&start).JumpIfUndefined(&start).JumpIfNotHole(
+      &start);
   // Perform an operation that returns boolean value to
   // generate JumpIfTrue/False
   builder.CompareOperation(Token::Value::EQ, reg, Strength::WEAK)
@@ -265,7 +266,8 @@ TEST_F(BytecodeArrayBuilderTest, AllBytecodesGenerated) {
       .CreateObjectLiteral(factory->NewFixedArray(2), 0, 0);
 
   // Longer jumps requiring ConstantWide operand
-  builder.Jump(&start).JumpIfNull(&start).JumpIfUndefined(&start);
+  builder.Jump(&start).JumpIfNull(&start).JumpIfUndefined(&start).JumpIfNotHole(
+      &start);
   // Perform an operation that returns boolean value to
   // generate JumpIfTrue/False
   builder.CompareOperation(Token::Value::EQ, reg, Strength::WEAK)