From 47c08f5f71fcdaeba3dff39fd292fcfe44c6d23a Mon Sep 17 00:00:00 2001 From: oth Date: Fri, 12 Feb 2016 07:24:01 -0800 Subject: [PATCH] [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 | 33 ++++++----- src/compiler/bytecode-graph-builder.h | 1 + src/interpreter/bytecode-array-builder.cc | 9 ++- src/interpreter/bytecode-array-builder.h | 3 +- src/interpreter/bytecode-generator.cc | 59 +++++++++---------- src/interpreter/bytecode-generator.h | 2 + src/interpreter/bytecodes.cc | 7 ++- src/interpreter/bytecodes.h | 4 +- src/interpreter/interpreter.cc | 34 +++++++---- test/mjsunit/mjsunit.status | 4 -- .../bytecode-array-builder-unittest.cc | 8 ++- 11 files changed, 87 insertions(+), 77 deletions(-) diff --git a/src/compiler/bytecode-graph-builder.cc b/src/compiler/bytecode-graph-builder.cc index 4cfd68d4ca..28810526d7 100644 --- a/src/compiler/bytecode-graph-builder.cc +++ b/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_) { diff --git a/src/compiler/bytecode-graph-builder.h b/src/compiler/bytecode-graph-builder.h index bf65b656dd..a8c5046abb 100644 --- a/src/compiler/bytecode-graph-builder.h +++ b/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); diff --git a/src/interpreter/bytecode-array-builder.cc b/src/interpreter/bytecode-array-builder.cc index f277d7ce18..3061b2215b 100644 --- a/src/interpreter/bytecode-array-builder.cc +++ b/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); diff --git a/src/interpreter/bytecode-array-builder.h b/src/interpreter/bytecode-array-builder.h index c4351b4977..97ef492934 100644 --- a/src/interpreter/bytecode-array-builder.h +++ b/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(); diff --git a/src/interpreter/bytecode-generator.cc b/src/interpreter/bytecode-generator.cc index da04906d20..f262dfdfc8 100644 --- a/src/interpreter/bytecode-generator.cc +++ b/src/interpreter/bytecode-generator.cc @@ -1698,9 +1698,7 @@ void BytecodeGenerator::BuildHoleCheckForVariableLoad(VariableMode mode, Handle 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 name) { +void BytecodeGenerator::BuildThrowReferenceError(Handle 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 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 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 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()); } } diff --git a/src/interpreter/bytecode-generator.h b/src/interpreter/bytecode-generator.h index 9fd4da63da..190160b7d1 100644 --- a/src/interpreter/bytecode-generator.h +++ b/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 name); void BuildThrowIfNotHole(Handle name); void BuildThrowReassignConstant(Handle name); + void BuildThrowReferenceError(Handle name); void BuildHoleCheckForVariableLoad(VariableMode mode, Handle name); void BuildHoleCheckForVariableAssignment(Variable* variable, Token::Value op); diff --git a/src/interpreter/bytecodes.cc b/src/interpreter/bytecodes.cc index 5296cc82b1..f58f37c770 100644 --- a/src/interpreter/bytecodes.cc +++ b/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; } diff --git a/src/interpreter/bytecodes.h b/src/interpreter/bytecodes.h index 51f9ec2e64..ba19fd7f16 100644 --- a/src/interpreter/bytecodes.h +++ b/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) \ diff --git a/src/interpreter/interpreter.cc b/src/interpreter/interpreter.cc index 418ba97b59..cd06baee50 100644 --- a/src/interpreter/interpreter.cc +++ b/src/interpreter/interpreter.cc @@ -1580,21 +1580,10 @@ void Interpreter::DoJumpIfUndefinedConstantWide( DoJumpIfUndefinedConstant(assembler); } -// JumpIfHole -// -// 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) { - 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); -} - // JumpIfNotHole // // Jump by number of bytes represented by an immediate operand if the object -// referenced by the accumulator is not the hole. +// referenced by the accumulator is the hole. void Interpreter::DoJumpIfNotHole(InterpreterAssembler* assembler) { Node* accumulator = __ GetAccumulator(); Node* the_hole_value = __ HeapConstant(isolate_->factory()->the_hole_value()); @@ -1602,6 +1591,27 @@ void Interpreter::DoJumpIfNotHole(InterpreterAssembler* assembler) { __ JumpIfWordNotEqual(accumulator, the_hole_value, relative_jump); } +// JumpIfNotHoleConstant +// +// 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* index = __ BytecodeOperandIdx(0); + Node* constant = __ LoadConstantPoolEntry(index); + Node* relative_jump = __ SmiUntag(constant); + __ JumpIfWordNotEqual(accumulator, the_hole_value, relative_jump); +} + +// JumpIfNotHoleConstantWide +// +// 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); diff --git a/test/mjsunit/mjsunit.status b/test/mjsunit/mjsunit.status index 05d3531348..5b13083e20 100644 --- a/test/mjsunit/mjsunit.status +++ b/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], diff --git a/test/unittests/interpreter/bytecode-array-builder-unittest.cc b/test/unittests/interpreter/bytecode-array-builder-unittest.cc index 842c9980aa..330aafa0e2 100644 --- a/test/unittests/interpreter/bytecode-array-builder-unittest.cc +++ b/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)