AuroraMiddleware/v8
1
0
Fork 0
Code Issues 2 Pull Requests Releases Wiki Activity

[maglev] Fix result regalloc clobbering inputs

Browse Source 
Consider the following

  * A ValueNode has inputs A and B
  * Input A has later uses, input B doesn't
  * The ValueNode's result must be in the same register as A

It can then happen that UpdateUses frees B, and the result allocation
emits a gap move from A's register to B's old register (now free) to
preserve the value of A when the ValueNode writes into its register.
This gap move is emmitted before the ValueNode start, which means that
it clobbers B.

Now, UpdateUses only clears registers _after_ node result allocation, so
that the known free registers are still the ones before updating uses.

Done naively, this would have bad consequences -- in the case where A
has no later uses, it would still force the regalloc to save its value
thinking that it is still live. So, this patch also introduces a concept
of "AllocationStage" where we're either allocating at the start or end
of a Node. Inputs are allocated at the start, results at the end. When
walking registers during an allocation, nodes whose lifetimes end at the
current node are considered to be dead at the "end" allocation stage,
and we are allowed to a) use their registers, and b) drop them without
preserving their value.

Bug: v8:7700
Change-Id: I5ca764ed04b12269f189577e81eb7e2a27cd1b09
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3625978
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Toon Verwaest <verwaest@chromium.org>
Cr-Commit-Position: refs/heads/main@{#80371}
This commit is contained in:
Leszek Swirski 2022-05-04 16:55:01 +02:00 committed by V8 LUCI CQ
parent 6bc97b4925
commit dc92fe0931
3 changed files with 159 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

127
src/maglev/maglev-regalloc.cc
View File

@ -346,6 +346,11 @@ void StraightForwardRegisterAllocator::UpdateUse(
if (!node->is_dead()) return;
if (FLAG_trace_maglev_regalloc) {
printing_visitor_->os()
<< "freeing " << PrintNodeLabel(graph_labeller(), node) << "\n";
}
// If a value is dead, make sure it's cleared.
FreeRegistersUsedBy(node);
@ -403,17 +408,30 @@ void StraightForwardRegisterAllocator::UpdateUse(
}
void StraightForwardRegisterAllocator::AllocateNode(Node* node) {
if (FLAG_trace_maglev_regalloc) {
printing_visitor_->os()
<< "Allocating " << PrintNodeLabel(graph_labeller(), node)
<< " inputs...\n";
}
for (Input& input : *node) AssignInput(input);
AssignTemporaries(node);
if (node->properties().can_eager_deopt()) {
UpdateUse(*node->eager_deopt_info());
}
for (Input& input : *node) UpdateUse(&input);
if (node->properties().is_call()) SpillAndClearRegisters();
// Allocate node output.
if (node->Is<ValueNode>()) AllocateNodeResult(node->Cast<ValueNode>());
if (node->Is<ValueNode>()) {
AllocateNodeResult(node->Cast<ValueNode>());
}
// Update uses only after allocating the node result. This order is necessary
// to avoid emitting input-clobbering gap moves during node result allocation
// -- a separate mechanism using AllocationStage ensures that the node result
// allocation is allowed to use the registers of nodes that are about to be
// dead.
if (node->properties().can_eager_deopt()) {
UpdateUse(*node->eager_deopt_info());
}
for (Input& input : *node) UpdateUse(&input);
// Lazy deopts are semantically after the node, so update them last.
if (node->properties().can_lazy_deopt()) {
@ -452,24 +470,28 @@ void StraightForwardRegisterAllocator::AllocateNodeResult(ValueNode* node) {
switch (operand.extended_policy()) {
case compiler::UnallocatedOperand::FIXED_REGISTER: {
Register r = Register::from_code(operand.fixed_register_index());
node->result().SetAllocated(ForceAllocate(r, node));
node->result().SetAllocated(
ForceAllocate(r, node, AllocationStage::kAtEnd));
break;
}
case compiler::UnallocatedOperand::MUST_HAVE_REGISTER:
node->result().SetAllocated(AllocateRegister(node));
node->result().SetAllocated(
AllocateRegister(node, AllocationStage::kAtEnd));
break;
case compiler::UnallocatedOperand::SAME_AS_INPUT: {
Input& input = node->input(operand.input_index());
node->result().SetAllocated(ForceAllocate(input, node));
node->result().SetAllocated(
ForceAllocate(input, node, AllocationStage::kAtEnd));
break;
}
case compiler::UnallocatedOperand::FIXED_FP_REGISTER: {
DoubleRegister r =
DoubleRegister::from_code(operand.fixed_register_index());
node->result().SetAllocated(ForceAllocate(r, node));
node->result().SetAllocated(
ForceAllocate(r, node, AllocationStage::kAtEnd));
break;
}
@ -494,7 +516,8 @@ void StraightForwardRegisterAllocator::AllocateNodeResult(ValueNode* node) {
template <typename RegisterT>
void StraightForwardRegisterAllocator::DropRegisterValue(
RegisterFrameState<RegisterT>& registers, RegisterT reg) {
RegisterFrameState<RegisterT>& registers, RegisterT reg,
AllocationStage stage) {
// The register should not already be free.
DCHECK(!registers.free().has(reg));
@ -507,6 +530,12 @@ void StraightForwardRegisterAllocator::DropRegisterValue(
// Return if the removed value already has another register or is spilled.
if (node->has_register() || node->is_spilled()) return;
// If we are at the end of the current node, and the last use of the given
// node is the current node, allow it to be dropped.
if (stage == AllocationStage::kAtEnd &&
node->live_range().end == node_it_->id())
return;
// Try to move the value to another register.
if (!registers.FreeIsEmpty()) {
RegisterT target_reg = registers.TakeFirstFree();
@ -530,12 +559,14 @@ void StraightForwardRegisterAllocator::DropRegisterValue(
Spill(node);
}
void StraightForwardRegisterAllocator::DropRegisterValue(Register reg) {
DropRegisterValue<Register>(general_registers_, reg);
void StraightForwardRegisterAllocator::DropRegisterValue(
Register reg, AllocationStage stage) {
DropRegisterValue<Register>(general_registers_, reg, stage);
}
void StraightForwardRegisterAllocator::DropRegisterValue(DoubleRegister reg) {
DropRegisterValue<DoubleRegister>(double_registers_, reg);
void StraightForwardRegisterAllocator::DropRegisterValue(
DoubleRegister reg, AllocationStage stage) {
DropRegisterValue<DoubleRegister>(double_registers_, reg, stage);
}
void StraightForwardRegisterAllocator::InitializeConditionalBranchRegisters(
@ -615,7 +646,8 @@ void StraightForwardRegisterAllocator::TryAllocateToInput(Phi* phi) {
// general register.
Register reg = input.AssignedGeneralRegister();
if (general_registers_.free().has(reg)) {
phi->result().SetAllocated(ForceAllocate(reg, phi));
phi->result().SetAllocated(
ForceAllocate(reg, phi, AllocationStage::kAtStart));
if (FLAG_trace_maglev_regalloc) {
printing_visitor_->Process(
phi, ProcessingState(compilation_info_, block_it_));
@ -669,7 +701,7 @@ void StraightForwardRegisterAllocator::AssignInput(Input& input) {
case compiler::UnallocatedOperand::FIXED_REGISTER: {
Register reg = Register::from_code(operand.fixed_register_index());
input.SetAllocated(ForceAllocate(reg, node));
input.SetAllocated(ForceAllocate(reg, node, AllocationStage::kAtStart));
break;
}
@ -677,14 +709,14 @@ void StraightForwardRegisterAllocator::AssignInput(Input& input) {
if (location.IsAnyRegister()) {
input.SetAllocated(location);
} else {
input.SetAllocated(AllocateRegister(node));
input.SetAllocated(AllocateRegister(node, AllocationStage::kAtStart));
}
break;
case compiler::UnallocatedOperand::FIXED_FP_REGISTER: {
DoubleRegister reg =
DoubleRegister::from_code(operand.fixed_register_index());
input.SetAllocated(ForceAllocate(reg, node));
input.SetAllocated(ForceAllocate(reg, node, AllocationStage::kAtStart));
break;
}
@ -759,11 +791,22 @@ void StraightForwardRegisterAllocator::AllocateSpillSlot(ValueNode* node) {
template <typename RegisterT>
void StraightForwardRegisterAllocator::FreeSomeRegister(
RegisterFrameState<RegisterT>& registers) {
RegisterFrameState<RegisterT>& registers, AllocationStage stage) {
if (FLAG_trace_maglev_regalloc) {
printing_visitor_->os() << "need to free a register... ";
}
int furthest_use = 0;
RegisterT best = RegisterT::no_reg();
for (RegisterT reg : registers.used()) {
ValueNode* value = registers.GetValue(reg);
// If we're freeing at the end of allocation, and the given register's value
// will already be dead after being used as an input to this node, allow
// and indeed prefer using this register.
if (stage == AllocationStage::kAtEnd &&
value->live_range().end == node_it_->id()) {
best = reg;
break;
}
// The cheapest register to clear is a register containing a value that's
// contained in another register as well.
if (value->num_registers() > 1) {
@ -776,26 +819,32 @@ void StraightForwardRegisterAllocator::FreeSomeRegister(
best = reg;
}
}
if (FLAG_trace_maglev_regalloc) {
printing_visitor_->os()
<< "chose " << best << " with next use " << furthest_use << "\n";
}
DCHECK(best.is_valid());
DropRegisterValue(registers, best);
DropRegisterValue(registers, best, stage);
registers.AddToFree(best);
}
void StraightForwardRegisterAllocator::FreeSomeGeneralRegister() {
return FreeSomeRegister(general_registers_);
void StraightForwardRegisterAllocator::FreeSomeGeneralRegister(
AllocationStage stage) {
return FreeSomeRegister(general_registers_, stage);
}
void StraightForwardRegisterAllocator::FreeSomeDoubleRegister() {
return FreeSomeRegister(double_registers_);
void StraightForwardRegisterAllocator::FreeSomeDoubleRegister(
AllocationStage stage) {
return FreeSomeRegister(double_registers_, stage);
}
compiler::AllocatedOperand StraightForwardRegisterAllocator::AllocateRegister(
ValueNode* node) {
ValueNode* node, AllocationStage stage) {
compiler::InstructionOperand allocation;
if (node->use_double_register()) {
if (double_registers_.FreeIsEmpty()) FreeSomeDoubleRegister();
if (double_registers_.FreeIsEmpty()) FreeSomeDoubleRegister(stage);
allocation = double_registers_.TryAllocateRegister(node);
} else {
if (general_registers_.FreeIsEmpty()) FreeSomeGeneralRegister();
if (general_registers_.FreeIsEmpty()) FreeSomeGeneralRegister(stage);
allocation = general_registers_.TryAllocateRegister(node);
}
DCHECK(allocation.IsAllocated());
@ -804,7 +853,8 @@ compiler::AllocatedOperand StraightForwardRegisterAllocator::AllocateRegister(
template <typename RegisterT>
compiler::AllocatedOperand StraightForwardRegisterAllocator::ForceAllocate(
RegisterFrameState<RegisterT>& registers, RegisterT reg, ValueNode* node) {
RegisterFrameState<RegisterT>& registers, RegisterT reg, ValueNode* node,
AllocationStage stage) {
if (registers.free().has(reg)) {
// If it's already free, remove it from the free list.
registers.RemoveFromFree(reg);
@ -813,7 +863,7 @@ compiler::AllocatedOperand StraightForwardRegisterAllocator::ForceAllocate(
node->GetMachineRepresentation(),
reg.code());
} else {
DropRegisterValue(registers, reg);
DropRegisterValue(registers, reg, stage);
}
#ifdef DEBUG
DCHECK(!registers.free().has(reg));
@ -825,23 +875,24 @@ compiler::AllocatedOperand StraightForwardRegisterAllocator::ForceAllocate(
}
compiler::AllocatedOperand StraightForwardRegisterAllocator::ForceAllocate(
Register reg, ValueNode* node) {
Register reg, ValueNode* node, AllocationStage stage) {
DCHECK(!node->use_double_register());
return ForceAllocate<Register>(general_registers_, reg, node);
return ForceAllocate<Register>(general_registers_, reg, node,
AllocationStage::kAtStart);
}
compiler::AllocatedOperand StraightForwardRegisterAllocator::ForceAllocate(
DoubleRegister reg, ValueNode* node) {
DoubleRegister reg, ValueNode* node, AllocationStage stage) {
DCHECK(node->use_double_register());
return ForceAllocate<DoubleRegister>(double_registers_, reg, node);
return ForceAllocate<DoubleRegister>(double_registers_, reg, node, stage);
}
compiler::AllocatedOperand StraightForwardRegisterAllocator::ForceAllocate(
const Input& input, ValueNode* node) {
const Input& input, ValueNode* node, AllocationStage stage) {
if (input.IsDoubleRegister()) {
return ForceAllocate(input.AssignedDoubleRegister(), node);
return ForceAllocate(input.AssignedDoubleRegister(), node, stage);
} else {
return ForceAllocate(input.AssignedGeneralRegister(), node);
return ForceAllocate(input.AssignedGeneralRegister(), node, stage);
}
}
@ -866,7 +917,7 @@ void StraightForwardRegisterAllocator::AssignTemporaries(NodeBase* node) {
// Make sure that any initially set temporaries are definitely free.
for (Register reg : initial_temporaries) {
if (general_registers_.free().has(reg)) continue;
DropRegisterValue(general_registers_, reg);
DropRegisterValue(general_registers_, reg, AllocationStage::kAtStart);
general_registers_.AddToFree(reg);
}
@ -875,7 +926,7 @@ void StraightForwardRegisterAllocator::AssignTemporaries(NodeBase* node) {
// Free extra registers if necessary.
for (int i = num_free_registers; i < num_temporaries_needed; ++i) {
FreeSomeGeneralRegister();
FreeSomeGeneralRegister(AllocationStage::kAtStart);
}
DCHECK_GE(general_registers_.free().Count(), num_temporaries_needed);

32
src/maglev/maglev-regalloc.h
View File

@ -91,8 +91,12 @@ class StraightForwardRegisterAllocator {
~StraightForwardRegisterAllocator();
private:
enum class AllocationStage { kAtStart, kAtEnd };
RegisterFrameState<Register> general_registers_;
RegisterFrameState<DoubleRegister> double_registers_;
RegList free_general_registers_before_node_;
DoubleRegList free_double_registers_before_node_;
struct SpillSlotInfo {
SpillSlotInfo(uint32_t slot_index, NodeIdT freed_at_position)
@ -142,24 +146,30 @@ class StraightForwardRegisterAllocator {
void FreeRegistersUsedBy(ValueNode* node);
template <typename RegisterT>
void FreeSomeRegister(RegisterFrameState<RegisterT>& registers);
void FreeSomeGeneralRegister();
void FreeSomeDoubleRegister();
void FreeSomeRegister(RegisterFrameState<RegisterT>& registers,
AllocationStage stage);
void FreeSomeGeneralRegister(AllocationStage stage);
void FreeSomeDoubleRegister(AllocationStage stage);
template <typename RegisterT>
void DropRegisterValue(RegisterFrameState<RegisterT>& registers,
RegisterT reg);
void DropRegisterValue(Register reg);
void DropRegisterValue(DoubleRegister reg);
RegisterT reg, AllocationStage stage);
void DropRegisterValue(Register reg, AllocationStage stage);
void DropRegisterValue(DoubleRegister reg, AllocationStage stage);
compiler::AllocatedOperand AllocateRegister(ValueNode* node);
compiler::AllocatedOperand AllocateRegister(ValueNode* node,
AllocationStage stage);
template <typename RegisterT>
compiler::AllocatedOperand ForceAllocate(
RegisterFrameState<RegisterT>& registers, RegisterT reg, ValueNode* node);
compiler::AllocatedOperand ForceAllocate(Register reg, ValueNode* node);
compiler::AllocatedOperand ForceAllocate(DoubleRegister reg, ValueNode* node);
compiler::AllocatedOperand ForceAllocate(const Input& input, ValueNode* node);
RegisterFrameState<RegisterT>& registers, RegisterT reg, ValueNode* node,
AllocationStage stage);
compiler::AllocatedOperand ForceAllocate(Register reg, ValueNode* node,
AllocationStage stage);
compiler::AllocatedOperand ForceAllocate(DoubleRegister reg, ValueNode* node,
AllocationStage stage);
compiler::AllocatedOperand ForceAllocate(const Input& input, ValueNode* node,
AllocationStage stage);
template <typename Function>
void ForEachMergePointRegisterState(

49
test/mjsunit/maglev/mutable-context-access.js
View File

@ -43,3 +43,52 @@ assertEquals(5, foo());
%OptimizeMaglevOnNextCall(foo);
assertEquals(5, foo());
function nested_factory_mutable() {
var x = 1;
x = 2;
return (function() {
var z = 3;
z = 4;
return function foo(){
// Add two values from different contexts to force an
// LdaImmutableCurrentContextSlot
return x+z;
}
})();
}
foo = nested_factory_mutable();
%PrepareFunctionForOptimization(foo);
assertEquals(6, foo());
assertEquals(6, foo());
%OptimizeMaglevOnNextCall(foo);
assertEquals(6, foo());
function nested_factory_immutable() {
var x = 1;
return (function() {
var z = 3;
z = 4;
return function foo(){
// Add two values from different contexts to force an
// LdaImmutableCurrentContextSlot
return x+z;
}
})();
}
foo = nested_factory_immutable();
%PrepareFunctionForOptimization(foo);
assertEquals(5, foo());
assertEquals(5, foo());
%OptimizeMaglevOnNextCall(foo);
assertEquals(5, foo());