Skip to content

V
V8
Commit eccbdde0 authored by mythria's avatar mythria Committed by Commit bot
Browse files
Options

[Interpreter] Removes assignment hazard scope. 

Removes assignment hazard scope. Reverts back to the naive scheme of
allocating a temporary for every variable load. It was decided to revert it
because the current implementation does not handle logical expressions,
ternary operators, visiting objects in named/keyed loads. Also, we wanted
to evaluate alternate approaches and choose one when we have a mechanism
to measure performance.

BUG=v8:4280
LOG=N

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

Cr-Commit-Position: refs/heads/master@{#33269}
parent ed21aa24
Hide whitespace changes
Inline Side-by-side
Showing with 1043 additions and 901 deletions
src/interpreter/bytecode-array-builder.cc
View file @ eccbdde0
......@@ -1155,12 +1155,6 @@ int BytecodeArrayBuilder::BorrowTemporaryRegisterNotInRange(int start_index,
}
int BytecodeArrayBuilder::AllocateAndBorrowTemporaryRegister() {
temporary_register_count_ += 1;
return last_temporary_register().index();
}
void BytecodeArrayBuilder::BorrowConsecutiveTemporaryRegister(int reg_index) {
DCHECK(free_temporaries_.find(reg_index) != free_temporaries_.end());
free_temporaries_.erase(reg_index);
......@@ -1639,13 +1633,6 @@ Register TemporaryRegisterScope::NewRegister() {
}
Register TemporaryRegisterScope::AllocateNewRegister() {
int allocated = builder_->AllocateAndBorrowTemporaryRegister();
allocated_.push_back(allocated);
return Register(allocated);
}
bool TemporaryRegisterScope::RegisterIsAllocatedInThisScope(
Register reg) const {
for (auto i = allocated_.begin(); i != allocated_.end(); i++) {
......
src/interpreter/bytecode-array-builder.h
View file @ eccbdde0
......@@ -306,7 +306,6 @@ class BytecodeArrayBuilder final {
// Temporary register management.
int BorrowTemporaryRegister();
int BorrowTemporaryRegisterNotInRange(int start_index, int end_index);
int AllocateAndBorrowTemporaryRegister();
void ReturnTemporaryRegister(int reg_index);
int PrepareForConsecutiveTemporaryRegisters(size_t count);
void BorrowConsecutiveTemporaryRegister(int reg_index);
......@@ -390,7 +389,6 @@ class TemporaryRegisterScope {
explicit TemporaryRegisterScope(BytecodeArrayBuilder* builder);
~TemporaryRegisterScope();
Register NewRegister();
Register AllocateNewRegister();
void PrepareForConsecutiveAllocations(size_t count);
Register NextConsecutiveRegister();
......
src/interpreter/bytecode-generator.cc
View file @ eccbdde0
......@@ -214,11 +214,12 @@ class BytecodeGenerator::ExpressionResultScope {
// VisitForRegisterValue allocates register in outer context.
return allocator_.NewRegister();
} else {
// We need this when allocating registers due to an Assignment hazard.
// It might be expensive to walk the full context chain and compute the
// list of consecutive reservations in the innerscopes. So allocates a
// new unallocated temporary register.
return allocator_.AllocateNewRegister();
// If it is required to allocate a register other than current or outer
// scopes, allocate a new temporary register. It might be expensive to
// walk the full context chain and compute the list of consecutive
// reservations in the innerscopes.
UNIMPLEMENTED();
return Register(-1);
}
}
......@@ -312,120 +313,6 @@ class BytecodeGenerator::RegisterResultScope final
};
BytecodeGenerator::AssignmentHazardHelper::AssignmentHazardHelper(
BytecodeGenerator* generator)
: generator_(generator),
alias_mappings_(generator->zone()),
aliased_locals_and_parameters_(generator->zone()),
execution_result_(nullptr),
scope_depth_(0) {}
void BytecodeGenerator::AssignmentHazardHelper::EnterScope() {
DCHECK_GE(scope_depth_, 0);
if (scope_depth_++ == 0) {
execution_result_ = generator_->execution_result();
}
}
void BytecodeGenerator::AssignmentHazardHelper::LeaveScope() {
DCHECK_GT(scope_depth_, 0);
if (--scope_depth_ == 0) {
DCHECK_EQ(execution_result_, generator_->execution_result());
RestoreAliasedLocalsAndParameters();
}
}
// Returns a register that a load instruction should use when
// loading from |reg|. This allows an alias for a modified version
// of |reg| to be used within a hazard regions.
MUST_USE_RESULT Register
BytecodeGenerator::AssignmentHazardHelper::GetRegisterForLoad(Register reg) {
if (scope_depth_ == 0) {
return reg;
} else {
// A load from |reg| is to be issued. The register is placed in
// the mappings table initially mapping to itself. Future stores
// will update the mapping with temporaries thus preserving the
// original register's value.
//
// NB This insert only updates the table if no mapping exists
// already (std::map::insert semantics).
auto insert_result =
alias_mappings_.insert(std::make_pair(reg.index(), reg.index()));
auto mapping = insert_result.first;
// Return the current alias for reg.
return Register(mapping->second);
}
}
// Returns a register that a store instruction should use when
// loading from |reg|. This allows an alias for a modified version
// of |reg| to be used within hazard regions.
MUST_USE_RESULT Register
BytecodeGenerator::AssignmentHazardHelper::GetRegisterForStore(Register reg) {
if (scope_depth_ == 0 ||
alias_mappings_.find(reg.index()) == alias_mappings_.end()) {
// If not in a hazard region or a load for this register has not
// occurred no mapping is necessary.
return reg;
} else {
// Storing to a register with 1 or more loads issued. The
// register is mapped to a temporary alias so we don't overwrite
// the lhs value, e.g. y = x + (x = 1); has a register for x on
// the lhs and needs a new register x' for the upcoming store on
// the rhs as the original x is an input to the add operation.
Register alias = execution_result_->NewRegister();
alias_mappings_[reg.index()] = alias.index();
if (generator_->builder()->RegisterIsParameterOrLocal(reg)) {
// Keep track of registers that need to be restored on exit
// from the assignment hazard region.
aliased_locals_and_parameters_.insert(reg.index());
}
return alias;
}
}
void BytecodeGenerator::AssignmentHazardHelper::
RestoreAliasedLocalsAndParameters() {
DCHECK(scope_depth_ == 0);
// Move temporary registers holding values for locals and
// parameters back into their local and parameter registers.
for (auto reg = aliased_locals_and_parameters_.begin();
reg != aliased_locals_and_parameters_.end(); reg++) {
auto mapping = alias_mappings_.find(*reg);
if (mapping != alias_mappings_.end()) {
generator_->builder()->MoveRegister(Register(mapping->second),
Register(*reg));
}
}
alias_mappings_.clear();
aliased_locals_and_parameters_.clear();
}
class BytecodeGenerator::AssignmentHazardScope final {
public:
explicit AssignmentHazardScope(BytecodeGenerator* generator)
: generator_(generator) {
generator_->assignment_hazard_helper()->EnterScope();
}
~AssignmentHazardScope() {
generator_->assignment_hazard_helper()->LeaveScope();
}
private:
BytecodeGenerator* generator_;
DISALLOW_COPY_AND_ASSIGN(AssignmentHazardScope);
};
BytecodeGenerator::BytecodeGenerator(Isolate* isolate, Zone* zone)
: isolate_(isolate),
zone_(zone),
......@@ -435,8 +322,7 @@ BytecodeGenerator::BytecodeGenerator(Isolate* isolate, Zone* zone)
globals_(0, zone),
execution_control_(nullptr),
execution_context_(nullptr),
execution_result_(nullptr),
assignment_hazard_helper_(this) {
execution_result_(nullptr) {
InitializeAstVisitor(isolate);
}
......@@ -708,6 +594,9 @@ void BytecodeGenerator::VisitWithStatement(WithStatement* stmt) {
void BytecodeGenerator::VisitSwitchStatement(SwitchStatement* stmt) {
// We need this scope because we visit for register values. We have to
// maintain a execution result scope where registers can be allocated.
EffectResultScope effect_results_scope(this);
ZoneList<CaseClause*>* clauses = stmt->cases();
SwitchBuilder switch_builder(builder(), clauses->length());
ControlScopeForBreakable scope(this, stmt, &switch_builder);
......@@ -1281,16 +1170,16 @@ void BytecodeGenerator::VisitVariableLoad(Variable* variable,
switch (variable->location()) {
case VariableLocation::LOCAL: {
Register source(Register(variable->index()));
source = assignment_hazard_helper()->GetRegisterForLoad(source);
execution_result()->SetResultInRegister(source);
builder()->LoadAccumulatorWithRegister(source);
execution_result()->SetResultInAccumulator();
break;
}
case VariableLocation::PARAMETER: {
// The parameter indices are shifted by 1 (receiver is variable
// index -1 but is parameter index 0 in BytecodeArrayBuilder).
Register source = builder()->Parameter(variable->index() + 1);
source = assignment_hazard_helper()->GetRegisterForLoad(source);
execution_result()->SetResultInRegister(source);
builder()->LoadAccumulatorWithRegister(source);
execution_result()->SetResultInAccumulator();
break;
}
case VariableLocation::GLOBAL:
......@@ -1358,8 +1247,6 @@ void BytecodeGenerator::VisitVariableAssignment(Variable* variable,
case VariableLocation::LOCAL: {
// TODO(rmcilroy): support const mode initialization.
Register destination(variable->index());
destination =
assignment_hazard_helper()->GetRegisterForStore(destination);
builder()->StoreAccumulatorInRegister(destination);
break;
}
......@@ -1367,9 +1254,6 @@ void BytecodeGenerator::VisitVariableAssignment(Variable* variable,
// The parameter indices are shifted by 1 (receiver is variable
// index -1 but is parameter index 0 in BytecodeArrayBuilder).
Register destination(builder()->Parameter(variable->index() + 1));
destination =
assignment_hazard_helper()->GetRegisterForStore(destination);
builder()->StoreAccumulatorInRegister(destination);
break;
}
......@@ -1970,14 +1854,6 @@ void BytecodeGenerator::VisitBinaryOperation(BinaryOperation* binop) {
void BytecodeGenerator::VisitCompareOperation(CompareOperation* expr) {
// The evaluation of binary comparison expressions has an assignment
// hazard because the lhs may be a variable that evaluates to a
// local or parameter and the rhs may modify that, e.g. y = x + (x = 1)
// To get a correct result the generator treats the inner assigment
// as being made to a temporary x' that is spilled on exit of the
// assignment hazard.
AssignmentHazardScope assignment_hazard_scope(this);
Register lhs = VisitForRegisterValue(expr->left());
VisitForAccumulatorValue(expr->right());
builder()->CompareOperation(expr->op(), lhs, language_mode_strength());
......@@ -1986,14 +1862,6 @@ void BytecodeGenerator::VisitCompareOperation(CompareOperation* expr) {
void BytecodeGenerator::VisitArithmeticExpression(BinaryOperation* expr) {
// The evaluation of binary arithmetic expressions has an assignment
// hazard because the lhs may be a variable that evaluates to a
// local or parameter and the rhs may modify that, e.g. y = x + (x = 1)
// To get a correct result the generator treats the inner assigment
// as being made to a temporary x' that is spilled on exit of the
// assignment hazard.
AssignmentHazardScope assignment_hazard_scope(this);
Register lhs = VisitForRegisterValue(expr->left());
VisitForAccumulatorValue(expr->right());
builder()->BinaryOperation(expr->op(), lhs, language_mode_strength());
......
src/interpreter/bytecode-generator.h
View file @ eccbdde0
......@@ -35,37 +35,6 @@ class BytecodeGenerator final : public AstVisitor {
class EffectResultScope;
class AccumulatorResultScope;
class RegisterResultScope;
class AssignmentHazardScope;
// Helper class that aliases locals and parameters when assignment
// hazards occur in binary expressions. For y = x + (x = 1) has an
// assignment hazard because the lhs evaluates to the register
// holding x and the rhs (x = 1) potentially updates x. When this
// hazard is detected, the rhs uses a temporary to hold the newer
// value of x while preserving the lhs for the binary expresion
// evaluation. The newer value is spilled to x at the end of the
// binary expression evaluation.
class AssignmentHazardHelper final {
public:
explicit AssignmentHazardHelper(BytecodeGenerator* generator);
MUST_USE_RESULT Register GetRegisterForLoad(Register reg);
MUST_USE_RESULT Register GetRegisterForStore(Register reg);
private:
friend class AssignmentHazardScope;
void EnterScope();
void LeaveScope();
void RestoreAliasedLocalsAndParameters();
BytecodeGenerator* generator_;
ZoneMap<int, int> alias_mappings_;
ZoneSet<int> aliased_locals_and_parameters_;
ExpressionResultScope* execution_result_;
int scope_depth_;
DISALLOW_COPY_AND_ASSIGN(AssignmentHazardHelper);
};
void MakeBytecodeBody();
Register NextContextRegister() const;
......@@ -149,9 +118,6 @@ class BytecodeGenerator final : public AstVisitor {
execution_result_ = execution_result;
}
ExpressionResultScope* execution_result() const { return execution_result_; }
inline AssignmentHazardHelper* assignment_hazard_helper() {
return &assignment_hazard_helper_;
}
ZoneVector<Handle<Object>>* globals() { return &globals_; }
inline LanguageMode language_mode() const;
......@@ -167,7 +133,6 @@ class BytecodeGenerator final : public AstVisitor {
ControlScope* execution_control_;
ContextScope* execution_context_;
ExpressionResultScope* execution_result_;
AssignmentHazardHelper assignment_hazard_helper_;
};
} // namespace interpreter
......
test/cctest/interpreter/test-bytecode-generator.cc
View file @ eccbdde0
This source diff could not be displayed because it is too large. You can view the blob instead.
test/mjsunit/mjsunit.status
View file @ eccbdde0
......@@ -793,6 +793,7 @@
'compiler/regress-const': [SKIP],
'compiler/regress-funarguments': [SKIP],
'compiler/regress-stacktrace-methods': [SKIP],
'compiler/regress-variable-liveness': [SKIP],
'compiler/rotate': [SKIP],
'compiler/safepoint': [SKIP],
'compiler/try-deopt': [SKIP],
......@@ -830,10 +831,6 @@
'math-min-max': [SKIP],
'messages': [SKIP],
'mirror-object': [SKIP],
'numops-fuzz-part1': [SKIP],
'numops-fuzz-part2': [SKIP],
'numops-fuzz-part3': [SKIP],
'numops-fuzz-part4': [SKIP],
'object-literal-gc': [SKIP],
'osr-elements-kind': [SKIP],
'property-load-across-eval': [SKIP],
......@@ -893,7 +890,6 @@
'regress/regress-269': [SKIP],
'regress/regress-2790': [SKIP],
'regress/regress-2825': [SKIP],
'regress/regress-286': [SKIP],
'regress/regress-298269': [SKIP],
'regress/regress-3135': [SKIP],
'regress/regress-3138': [SKIP],
......@@ -957,7 +953,6 @@
'regress/regress-568765': [SKIP],
'regress/regress-580': [SKIP],
'regress/regress-618': [SKIP],
'regress/regress-643': [SKIP],
'regress/regress-69': [SKIP],
'regress/regress-70066': [SKIP],
'regress/regress-747': [SKIP],
......@@ -1056,6 +1051,7 @@
'regress/regress-transcendental': [SKIP],
'regress/regress-typedarray-length': [SKIP],
'regress/splice-missing-wb': [SKIP],
'setter-on-constructor-prototype': [SKIP],
'shift-for-integer-div': [SKIP],
'simple-constructor': [SKIP],
'sparse-array-reverse': [SKIP],
......@@ -1067,7 +1063,6 @@
'string-natives': [SKIP],
'string-replace-with-empty': [SKIP],
'string-slices': [SKIP],
'switch-opt': [SKIP],
'tools/profile': [SKIP],
'tools/profviz': [SKIP],
'try-finally-continue': [SKIP],
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment