Commit 75669792 authored by Shu-yu Guo's avatar Shu-yu Guo Committed by V8 LUCI CQ

Revert "[wasm][liftoff] Spill regs for multi-value merges"

This reverts commit d9e1f2ae.

Reason for revert: Linux test failures: https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Linux/45960/overview

Original change's description:
> [wasm][liftoff] Spill regs for multi-value merges
>
> If there is more than one value in the merge region, a stack-to-stack
> move can overwrite the source of a stack-to-register move. To avoid
> this, spill all registers.
>
> R=​clemensb@chromium.org
>
> Bug: chromium:1299183
> Change-Id: I10495434d0a18c9072ee3882e00a687edd8c592a
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3523044
> Reviewed-by: Clemens Backes <clemensb@chromium.org>
> Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#79584}

Bug: chromium:1299183
Change-Id: I465129695cfc1c5678923f7eefe5b91e31383798
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3546745
Auto-Submit: Shu-yu Guo <syg@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Shu-yu Guo <syg@chromium.org>
Owners-Override: Shu-yu Guo <syg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79585}
parent d9e1f2ae
...@@ -1164,22 +1164,12 @@ void LiftoffAssembler::MoveStackValue(uint32_t dst_offset, uint32_t src_offset, ...@@ -1164,22 +1164,12 @@ void LiftoffAssembler::MoveStackValue(uint32_t dst_offset, uint32_t src_offset,
DCHECK_EQ(0, element_size_bytes(kind) % kSystemPointerSize); DCHECK_EQ(0, element_size_bytes(kind) % kSystemPointerSize);
int words = element_size_bytes(kind) / kSystemPointerSize; int words = element_size_bytes(kind) / kSystemPointerSize;
DCHECK_LE(1, words); DCHECK_LE(1, words);
// Make sure we move the words in the correct order in case there is an do {
// overlap between src and dst. liftoff::MoveStackValue(this, liftoff::GetStackSlot(src_offset),
if (src_offset < dst_offset) { liftoff::GetStackSlot(dst_offset));
do { dst_offset -= kSystemPointerSize;
liftoff::MoveStackValue(this, liftoff::GetStackSlot(src_offset), src_offset -= kSystemPointerSize;
liftoff::GetStackSlot(dst_offset)); } while (--words);
dst_offset -= kSystemPointerSize;
src_offset -= kSystemPointerSize;
} while (--words);
} else {
while (words--) {
liftoff::MoveStackValue(
this, liftoff::GetStackSlot(src_offset - words * kSystemPointerSize),
liftoff::GetStackSlot(dst_offset - words * kSystemPointerSize));
}
}
} }
void LiftoffAssembler::Move(Register dst, Register src, ValueKind kind) { void LiftoffAssembler::Move(Register dst, Register src, ValueKind kind) {
......
...@@ -391,10 +391,6 @@ enum MergeAllowConstants : bool { ...@@ -391,10 +391,6 @@ enum MergeAllowConstants : bool {
kConstantsAllowed = true, kConstantsAllowed = true,
kConstantsNotAllowed = false kConstantsNotAllowed = false
}; };
enum MergeAllowRegisters : bool {
kRegistersAllowed = true,
kRegistersNotAllowed = false
};
enum ReuseRegisters : bool { enum ReuseRegisters : bool {
kReuseRegisters = true, kReuseRegisters = true,
kNoReuseRegisters = false kNoReuseRegisters = false
...@@ -403,7 +399,6 @@ void InitMergeRegion(LiftoffAssembler::CacheState* state, ...@@ -403,7 +399,6 @@ void InitMergeRegion(LiftoffAssembler::CacheState* state,
const VarState* source, VarState* target, uint32_t count, const VarState* source, VarState* target, uint32_t count,
MergeKeepStackSlots keep_stack_slots, MergeKeepStackSlots keep_stack_slots,
MergeAllowConstants allow_constants, MergeAllowConstants allow_constants,
MergeAllowRegisters allow_registers,
ReuseRegisters reuse_registers, LiftoffRegList used_regs) { ReuseRegisters reuse_registers, LiftoffRegList used_regs) {
RegisterReuseMap register_reuse_map; RegisterReuseMap register_reuse_map;
for (const VarState* source_end = source + count; source < source_end; for (const VarState* source_end = source + count; source < source_end;
...@@ -414,21 +409,18 @@ void InitMergeRegion(LiftoffAssembler::CacheState* state, ...@@ -414,21 +409,18 @@ void InitMergeRegion(LiftoffAssembler::CacheState* state,
continue; continue;
} }
base::Optional<LiftoffRegister> reg; base::Optional<LiftoffRegister> reg;
if (allow_registers) { // First try: Keep the same register, if it's free.
// First try: Keep the same register, if it's free. if (source->is_reg() && state->is_free(source->reg())) {
if (source->is_reg() && state->is_free(source->reg())) { reg = source->reg();
reg = source->reg(); }
} // Second try: Use the same register we used before (if we reuse registers).
// Second try: Use the same register we used before (if we reuse if (!reg && reuse_registers) {
// registers). reg = register_reuse_map.Lookup(source->reg());
if (!reg && reuse_registers) { }
reg = register_reuse_map.Lookup(source->reg()); // Third try: Use any free register.
} RegClass rc = reg_class_for(source->kind());
// Third try: Use any free register. if (!reg && state->has_unused_register(rc, used_regs)) {
RegClass rc = reg_class_for(source->kind()); reg = state->unused_register(rc, used_regs);
if (!reg && state->has_unused_register(rc, used_regs)) {
reg = state->unused_register(rc, used_regs);
}
} }
if (!reg) { if (!reg) {
// No free register; make this a stack slot. // No free register; make this a stack slot.
...@@ -477,17 +469,9 @@ void LiftoffAssembler::CacheState::InitMerge(const CacheState& source, ...@@ -477,17 +469,9 @@ void LiftoffAssembler::CacheState::InitMerge(const CacheState& source,
for (auto& src : base::VectorOf(source_begin, num_locals)) { for (auto& src : base::VectorOf(source_begin, num_locals)) {
if (src.is_reg()) used_regs.set(src.reg()); if (src.is_reg()) used_regs.set(src.reg());
} }
// If there is more than one operand in the merge region, a stack-to-stack for (auto& src :
// move can interfere with a register reload, which would not be handled base::VectorOf(source_begin + stack_base + discarded, arity)) {
// correctly by the StackTransferRecipe. To avoid this, spill all registers in if (src.is_reg()) used_regs.set(src.reg());
// this region.
MergeAllowRegisters allow_registers =
arity <= 1 ? kRegistersAllowed : kRegistersNotAllowed;
if (allow_registers) {
for (auto& src :
base::VectorOf(source_begin + stack_base + discarded, arity)) {
if (src.is_reg()) used_regs.set(src.reg());
}
} }
// Initialize the merge region. If this region moves, try to turn stack slots // Initialize the merge region. If this region moves, try to turn stack slots
...@@ -496,8 +480,7 @@ void LiftoffAssembler::CacheState::InitMerge(const CacheState& source, ...@@ -496,8 +480,7 @@ void LiftoffAssembler::CacheState::InitMerge(const CacheState& source,
discarded == 0 ? kKeepStackSlots : kTurnStackSlotsIntoRegisters; discarded == 0 ? kKeepStackSlots : kTurnStackSlotsIntoRegisters;
InitMergeRegion(this, source_begin + stack_base + discarded, InitMergeRegion(this, source_begin + stack_base + discarded,
target_begin + stack_base, arity, keep_merge_stack_slots, target_begin + stack_base, arity, keep_merge_stack_slots,
kConstantsNotAllowed, allow_registers, kNoReuseRegisters, kConstantsNotAllowed, kNoReuseRegisters, used_regs);
used_regs);
// Shift spill offsets down to keep slots contiguous. // Shift spill offsets down to keep slots contiguous.
int offset = stack_base == 0 ? StaticStackFrameSize() int offset = stack_base == 0 ? StaticStackFrameSize()
: source.stack_state[stack_base - 1].offset(); : source.stack_state[stack_base - 1].offset();
...@@ -510,8 +493,7 @@ void LiftoffAssembler::CacheState::InitMerge(const CacheState& source, ...@@ -510,8 +493,7 @@ void LiftoffAssembler::CacheState::InitMerge(const CacheState& source,
// Initialize the locals region. Here, stack slots stay stack slots (because // Initialize the locals region. Here, stack slots stay stack slots (because
// they do not move). Try to keep register in registers, but avoid duplicates. // they do not move). Try to keep register in registers, but avoid duplicates.
InitMergeRegion(this, source_begin, target_begin, num_locals, kKeepStackSlots, InitMergeRegion(this, source_begin, target_begin, num_locals, kKeepStackSlots,
kConstantsNotAllowed, kRegistersAllowed, kNoReuseRegisters, kConstantsNotAllowed, kNoReuseRegisters, used_regs);
used_regs);
// Consistency check: All the {used_regs} are really in use now. // Consistency check: All the {used_regs} are really in use now.
DCHECK_EQ(used_regs, used_registers & used_regs); DCHECK_EQ(used_regs, used_registers & used_regs);
...@@ -521,7 +503,7 @@ void LiftoffAssembler::CacheState::InitMerge(const CacheState& source, ...@@ -521,7 +503,7 @@ void LiftoffAssembler::CacheState::InitMerge(const CacheState& source,
// source region, ensure to use the same register twice in the target region. // source region, ensure to use the same register twice in the target region.
InitMergeRegion(this, source_begin + num_locals, target_begin + num_locals, InitMergeRegion(this, source_begin + num_locals, target_begin + num_locals,
stack_depth, kKeepStackSlots, kConstantsAllowed, stack_depth, kKeepStackSlots, kConstantsAllowed,
kRegistersAllowed, kReuseRegisters, used_regs); kReuseRegisters, used_regs);
} }
void LiftoffAssembler::CacheState::Steal(const CacheState& source) { void LiftoffAssembler::CacheState::Steal(const CacheState& source) {
...@@ -803,7 +785,7 @@ void LiftoffAssembler::MergeStackWith(CacheState& target, uint32_t arity, ...@@ -803,7 +785,7 @@ void LiftoffAssembler::MergeStackWith(CacheState& target, uint32_t arity,
transfers.TransferStackSlot(target.stack_state[target_stack_base + i], transfers.TransferStackSlot(target.stack_state[target_stack_base + i],
cache_state_.stack_state[stack_base + i]); cache_state_.stack_state[stack_base + i]);
DCHECK(!SlotInterference( DCHECK(!SlotInterference(
target.stack_state[target_stack_base + i], target.stack_state[i],
base::VectorOf(cache_state_.stack_state.data() + stack_base + i + 1, base::VectorOf(cache_state_.stack_state.data() + stack_base + i + 1,
arity - i - 1))); arity - i - 1)));
} }
......
This diff is collapsed.
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